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

["Albecki, Mateusz" <mateusz.albecki@intel.com>]

I will write a test code and update the results in next patch series.

Regarding the buffer data port read. Reading the spec I am not entirely sure on the behavior of the host when the data transfer is not aligned to DWORD boundary. I will test it with width set to 32bit and if that works I will fix it in v2.

Thanks,
Mateusz
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, February 5, 2020 4:16 AM
> To: devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
> Cc: Marcin Wojtas <mw@semihalf.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix
> PIO transfer mode
> 
> > -----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.
> >
> >
> > 
> 

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

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.