Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode

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

> -----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 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO
> transfer mode
> 
> Current driver does not support PIO transfer mode for
> commands other then tuning. This change adds the code
> to transfer PIO data.


Hello Mateusz,

Try to provide some feedbacks before I can test the patch.

One test request, is it possible for you to test the asynchronous transfer for
the PIO mode?

A possible method can be using an UEFI application to locate the BlockIO 2
protocol from a specific SD or eMMC device (which forced to PIO transfer mode).
And test with the WriteBlocksEx() & ReadBlocksEx() services to see if the RW
is successful.

Also, one more inline comment below:


> 
> 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/SdMmcPciHcDxe.h |   3 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 113
> +++++++++++++++++----
>  2 files changed, 95 insertions(+), 21 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index 15b7d12596..fd89306fab 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -157,6 +157,9 @@ typedef struct {
>    UINT64                              Timeout;
>    UINT32                              Retries;
> 
> +  BOOLEAN                             PioModeTransferCompleted;
> +  UINT32                              PioBlockIndex;
> +
>    SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
>    SD_MMC_HC_ADMA_64_V3_DESC_LINE      *Adma64V3Desc;
>    SD_MMC_HC_ADMA_64_V4_DESC_LINE      *Adma64V4Desc;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 480a1664ea..43703974f7 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -1711,6 +1711,8 @@ SdMmcPrintTrb (
>    DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb-
> >CommandComplete));
>    DEBUG ((DebugLevel, "Timeout: %d\n", Trb->Timeout));
>    DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
> +  DEBUG ((DebugLevel, "PioModeTransferCompleted: %d\n", Trb-
> >PioModeTransferCompleted));
> +  DEBUG ((DebugLevel, "PioBlockIndex: %d\n", Trb->PioBlockIndex));
>    DEBUG ((DebugLevel, "Adma32Desc: %X\n", Trb->Adma32Desc));
>    DEBUG ((DebugLevel, "Adma64V3Desc: %X\n", Trb->Adma64V3Desc));
>    DEBUG ((DebugLevel, "Adma64V4Desc: %X\n", Trb->Adma64V4Desc));
> @@ -1762,6 +1764,8 @@ SdMmcCreateTrb (
>    Trb->CommandComplete = FALSE;
>    Trb->Timeout   = Packet->Timeout;
>    Trb->Retries   = SD_MMC_TRB_RETRIES;
> +  Trb->PioModeTransferCompleted = FALSE;
> +  Trb->PioBlockIndex = 0;
>    Trb->Private   = Private;
> 
>    if ((Packet->InTransferLength != 0) && (Packet->InDataBuffer != NULL)) {
> @@ -2447,6 +2451,85 @@ SdMmcCheckCommandComplete (
>    return EFI_NOT_READY;
>  }
> 
> +/**
> +  Transfers data from card using PIO method.
> +
> +  @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   PIO transfer completed successfully.
> +  @retval EFI_NOT_READY PIO transfer completion still pending.
> +  @retval Others        PIO transfer failed to complete.
> +**/
> +EFI_STATUS
> +SdMmcTransferDataWithPio (
> +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN SD_MMC_HC_TRB           *Trb,
> +  IN UINT16                  IntStatus
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT16      Data16;
> +  UINT32      BlockCount;
> +
> +  BlockCount = (Trb->DataLen / Trb->BlockSize);
> +  if (Trb->DataLen % Trb->BlockSize != 0) {
> +    BlockCount += 1;
> +  }
> +
> +  if (Trb->PioBlockIndex >= BlockCount) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  if (Trb->Read) {
> +    if ((IntStatus & BIT5) == 0) {
> +      return EFI_NOT_READY;
> +    }
> +    Data16 = BIT5;
> +    SdMmcHcRwMmio (Private->PciIo, Trb->Slot,
> SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (Data16), &Data16);
> +
> +    Status = Private->PciIo->Mem.Read (
> +               Private->PciIo,
> +               EfiPciIoWidthFifoUint8,
> +               Trb->Slot,
> +               SD_MMC_HC_BUF_DAT_PORT,
> +               Trb->BlockSize,
> +               (VOID*)((UINT8*)Trb->Data + (Trb->BlockSize * Trb-
> >PioBlockIndex))
> +               );


The read (write) process will be:

1. Wait for the Buffer Read (Write) Ready to set;
2. Clear the Buffer Read (Write) Ready bit;
3. Access the Buffer Data Port register 'Trb->BlockSize' times, each time
   consuming 1 byte to get all the data in a block.

Since we are accessing the Buffer Data Port register, same BAR offset, so
'EfiPciIoWidthFifoUint8' is used here.

Is my understanding correct?

If so, I am thinking is it less efficient during the read/write of the data?
Since the Buffer Data Port register is 4-byte in width, data can be accessed
at most 4 bytes a time. Not sure if doing so can save time for the PIO transfer.

Best Regards,
Hao Wu


> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    Trb->PioBlockIndex++;
> +  } else {
> +    if ((IntStatus & BIT4) == 0) {
> +      return EFI_NOT_READY;
> +    }
> +    Data16 = BIT4;
> +    SdMmcHcRwMmio (Private->PciIo, Trb->Slot,
> SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (Data16), &Data16);
> +
> +    Status = Private->PciIo->Mem.Write (
> +               Private->PciIo,
> +               EfiPciIoWidthFifoUint8,
> +               Trb->Slot,
> +               SD_MMC_HC_BUF_DAT_PORT,
> +               Trb->BlockSize,
> +               (VOID*)((UINT8*)Trb->Data + (Trb->BlockSize * Trb-
> >PioBlockIndex))
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    Trb->PioBlockIndex++;
> +  }
> +
> +  if (Trb->PioBlockIndex >= BlockCount) {
> +    Trb->PioModeTransferCompleted = TRUE;
> +    return EFI_SUCCESS;
> +  } else {
> +    return EFI_NOT_READY;
> +  }
> +}
> +
>  /**
>    Update the SDMA address on the SDMA buffer boundary interrupt.
> 
> @@ -2531,6 +2614,13 @@ SdMmcCheckDataTransfer (
>      return Status;
>    }
> 
> +  if (Trb->Mode == SdMmcPioMode && !Trb->PioModeTransferCompleted) {
> +    Status = SdMmcTransferDataWithPio (Private, Trb, IntStatus);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  }
> +
>    if ((Trb->Mode == SdMmcSdmaMode) && ((IntStatus & BIT3) != 0)) {
>      Data16 = BIT3;
>      Status = SdMmcHcRwMmio (
> @@ -2573,7 +2663,6 @@ SdMmcCheckTrbResult (
>    EFI_STATUS                          Status;
>    EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
>    UINT16                              IntStatus;
> -  UINT32                              PioLength;
> 
>    Packet  = Trb->Packet;
>    //
> @@ -2609,26 +2698,8 @@ SdMmcCheckTrbResult (
>         (Packet->SdMmcCmdBlk->CommandIndex ==
> EMMC_SEND_TUNING_BLOCK)) ||
>        ((Private->Slot[Trb->Slot].CardType == SdCardType) &&
>         (Packet->SdMmcCmdBlk->CommandIndex ==
> SD_SEND_TUNING_BLOCK))) {
> -    //
> -    // When performing tuning procedure (Execute Tuning is set to 1) through
> PIO mode,
> -    // wait Buffer Read Ready bit of Normal Interrupt Status Register to be 1.
> -    // Refer to SD Host Controller Simplified Specification 3.0 figure 2-29 for
> details.
> -    //
> -    if ((IntStatus & BIT5) == BIT5) {
> -      //
> -      // Clear Buffer Read Ready interrupt at first.
> -      //
> -      IntStatus = BIT5;
> -      SdMmcHcRwMmio (Private->PciIo, Trb->Slot,
> SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (IntStatus), &IntStatus);
> -      //
> -      // Read data out from Buffer Port register
> -      //
> -      for (PioLength = 0; PioLength < Trb->DataLen; PioLength += 4) {
> -        SdMmcHcRwMmio (Private->PciIo, Trb->Slot,
> SD_MMC_HC_BUF_DAT_PORT, TRUE, 4, (UINT8*)Trb->Data + PioLength);
> -      }
> -      Status = EFI_SUCCESS;
> -      goto Done;
> -    }
> +    Status = SdMmcTransferDataWithPio (Private, Trb, IntStatus);
> +    goto Done;
>    }
> 
>    if (!Trb->CommandComplete) {
> --
> 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.
> 
> 
>