[PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

[PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

[Albecki, Mateusz]

This patch series aims to refactor command processing to achieve following

- Trace the failing TRB packets to see what commands are failing and for what reasons
- Get the response data even if data transfer timed out to allow easier debugging
- Fix the PIO mode which is currently completely broken.

Changes in v2:
- Moved verbose packet prints after the command is finished to capture the successfull command response
- Fixed the debug prints
- PIO data will be moved with width matching the alignment of the block size. For majority of transfers that means UINT32 width.

Tests performed:
- Each patch in the series has passed boot from eMMC with ADMAv3 data transfer mode
- SDMA based boot has been tested with the full patch series
- PIO based boot has been tested with the full patch series
- PIO based data transfer has been additionally tested by creating and modyfing a file in EFI shell
- Tested async PIO transfer - results below

Async test results:
I've tested a simple async write and then readback from the eMMMC device using block IO v2. I have observed
that while the requests are processed correctly by the eMMC driver as soon as they finish platform will sometimes
mibehave. I've tested async without my changes and the strange behavior reproduces. Right now I am suspecting there
is some problem either in the EDK2 core that performs async or in the platform specific code. It is also possible that
I coded the Async BlockIo incorrectly although the test code was rather simple. Additionally I was able
to observe that on many eMMC controllers PIO based data transfer is broken. I was only able to find one platform
that supported PIO. Detailed observation below

Platform 1 (PIO working).
- Test code was able to perform async write to eMMC LBA 0
- As soon as the callback is called CPU exception happens
- After reboot test code was able to perform async read
- As soon as the callback is called CPU exception happens
- After reboot sync read was able to confirm that data matches what was written by async write

Platform2 (PIO is returning trash data - all 0xAF)
- Test code was able to perform async write(although it didn't realyl came through to the device)
- After write finished test code was able to perform async read(again all data was 0xAF but the logic in the driver works)

Platform2 (again PIO is returning trash data)
- Test code scheduled 5 async writes. 2 writes finished and after that CPU exception was signaled.

Platform3(also trash data from PIO)
- Test code scheduled one async write as soon as it was done platform rebooted

I didn't want to spend any more time debugging this issue as I think it would turn into the platform debug and what I
observed gave me some confidence that PIO in async is generally working.

All tests were performed with eMMC in HS400 @200MHz clock frequency.

For easier review & integration patch has been pushed here:
Whole series: https://github.com/malbecki/edk2/tree/emmc_transfer_refactor
Whole series + SDMA force code(test 3): https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_sdma
Whole series + PIO force code(test 4): https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_pio

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>

Mateusz Albecki (4):
  MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
  MdeModulePkg/SdMmcPciHcDxe: Read response on command completion
  MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
  MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode

 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 521 ++++++++++++++++-----
 2 files changed, 417 insertions(+), 108 deletions(-)

-- 
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.

[PATCHv2 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces

[Albecki, Mateusz]

To allow for easier debug of failing commands we
have added a capability to print TRB and command
packet when we start execution of the TRB(on
DEBUG_VERBOSE level) and when the TRB failed to
execute correctly(on DEBUG_ERROR level). Additionally
we will also print error interrupt status and interrupt
status register on failed SD command.

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 | 87 ++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index b05c818462..9bf9963848 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1643,6 +1643,82 @@ BuildAdmaDescTable (
   return EFI_SUCCESS;
 }
 
+/**
+  Prints the contents of the command packet to the debug port.
+
+  @param[in] DebugLevel  Debug level at which the packet should be printed.
+  @param[in] Packet      Pointer to packet to print.
+**/
+VOID
+SdMmcPrintPacket (
+  IN UINT32                               DebugLevel,
+  IN EFI_SD_MMC_PASS_THRU_COMMAND_PACKET  *Packet
+  )
+{
+  if (Packet == NULL) {
+    return;
+  }
+
+  DEBUG ((DebugLevel, "Printing EFI_SD_MMC_PASS_THRU_COMMAND_PACKET\n"));
+  if (Packet->SdMmcCmdBlk != NULL) {
+    DEBUG ((DebugLevel, "Command index: %d, argument: %X\n", Packet->SdMmcCmdBlk->CommandIndex, Packet->SdMmcCmdBlk->CommandArgument));
+    DEBUG ((DebugLevel, "Command type: %d, response type: %d\n", Packet->SdMmcCmdBlk->CommandType, Packet->SdMmcCmdBlk->ResponseType));
+  }
+  if (Packet->SdMmcStatusBlk != NULL) {
+    DEBUG ((DebugLevel, "Response 0: %X, 1: %X, 2: %X, 3: %X\n",
+                           Packet->SdMmcStatusBlk->Resp0,
+                           Packet->SdMmcStatusBlk->Resp1,
+                           Packet->SdMmcStatusBlk->Resp2,
+                           Packet->SdMmcStatusBlk->Resp3
+                           ));
+  }
+  DEBUG ((DebugLevel, "Timeout: %ld\n", Packet->Timeout));
+  DEBUG ((DebugLevel, "InDataBuffer: %p\n", Packet->InDataBuffer));
+  DEBUG ((DebugLevel, "OutDataBuffer: %p\n", Packet->OutDataBuffer));
+  DEBUG ((DebugLevel, "InTransferLength: %d\n", Packet->InTransferLength));
+  DEBUG ((DebugLevel, "OutTransferLength: %d\n", Packet->OutTransferLength));
+  DEBUG ((DebugLevel, "TransactionStatus: %r\n", Packet->TransactionStatus));
+}
+
+/**
+  Prints the contents of the TRB to the debug port.
+
+  @param[in] DebugLevel  Debug level at which the TRB should be printed.
+  @param[in] Trb         Pointer to the TRB structure.
+**/
+VOID
+SdMmcPrintTrb (
+  IN UINT32         DebugLevel,
+  IN SD_MMC_HC_TRB  *Trb
+  )
+{
+  if (Trb == NULL) {
+    return;
+  }
+
+  DEBUG ((DebugLevel, "Printing SD_MMC_HC_TRB\n"));
+  DEBUG ((DebugLevel, "Slot: %d\n", Trb->Slot));
+  DEBUG ((DebugLevel, "BlockSize: %d\n", Trb->BlockSize));
+  DEBUG ((DebugLevel, "Data: %p\n", Trb->Data));
+  DEBUG ((DebugLevel, "DataLen: %d\n", Trb->DataLen));
+  DEBUG ((DebugLevel, "Read: %d\n", Trb->Read));
+  DEBUG ((DebugLevel, "DataPhy: %lX\n", Trb->DataPhy));
+  DEBUG ((DebugLevel, "DataMap: %p\n", Trb->DataMap));
+  DEBUG ((DebugLevel, "Mode: %d\n", Trb->Mode));
+  DEBUG ((DebugLevel, "AdmaLengthMode: %d\n", Trb->AdmaLengthMode));
+  DEBUG ((DebugLevel, "Event: %p\n", Trb->Event));
+  DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
+  DEBUG ((DebugLevel, "Timeout: %ld\n", Trb->Timeout));
+  DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
+  DEBUG ((DebugLevel, "Adma32Desc: %p\n", Trb->Adma32Desc));
+  DEBUG ((DebugLevel, "Adma64V3Desc: %p\n", Trb->Adma64V3Desc));
+  DEBUG ((DebugLevel, "Adma64V4Desc: %p\n", Trb->Adma64V4Desc));
+  DEBUG ((DebugLevel, "AdmaMap: %p\n", Trb->AdmaMap));
+  DEBUG ((DebugLevel, "AdmaPages: %X\n", Trb->AdmaPages));
+
+  SdMmcPrintPacket (DebugLevel, Trb->Packet);
+}
+
 /**
   Create a new TRB for the SD/MMC cmd request.
 
@@ -2235,6 +2311,10 @@ SdMmcCheckAndRecoverErrors (
     return Status;
   }
 
+  DEBUG ((DEBUG_ERROR, "Error reported by SDHCI\n"));
+  DEBUG ((DEBUG_ERROR, "Interrupt status = %X\n", IntStatus));
+  DEBUG ((DEBUG_ERROR, "Error interrupt status = %X\n", ErrIntStatus));
+
   //
   // If the data timeout error is reported
   // but data transfer is signaled as completed we
@@ -2438,6 +2518,13 @@ Done:
 
   if (Status != EFI_NOT_READY) {
     SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "TRB failed with %r\n", Status));
+      SdMmcPrintTrb (DEBUG_ERROR, Trb);
+    } else {
+      DEBUG ((DEBUG_VERBOSE, "TRB success\n"));
+      SdMmcPrintTrb (DEBUG_VERBOSE, Trb);
+    }
   }
 
   return 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.

[PATCHv2 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion

[Albecki, Mateusz]

SdMmcPciHcDxe driver used to read response only after
command and data transfer completed. According to SDHCI
specification response data is ready after the command
complete status is set by the host controller. Getting
the response data early will help debugging the cases
when command completed but data transfer timed out.

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 |   1 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 201 +++++++++++++++------
 2 files changed, 144 insertions(+), 58 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 5bc3577ba2..15b7d12596 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -153,6 +153,7 @@ typedef struct {
 
   EFI_EVENT                           Event;
   BOOLEAN                             Started;
+  BOOLEAN                             CommandComplete;
   UINT64                              Timeout;
   UINT32                              Retries;
 
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 9bf9963848..38a8099426 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1708,6 +1708,7 @@ SdMmcPrintTrb (
   DEBUG ((DebugLevel, "AdmaLengthMode: %d\n", Trb->AdmaLengthMode));
   DEBUG ((DebugLevel, "Event: %p\n", Trb->Event));
   DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
+  DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb->CommandComplete));
   DEBUG ((DebugLevel, "Timeout: %ld\n", Trb->Timeout));
   DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
   DEBUG ((DebugLevel, "Adma32Desc: %p\n", Trb->Adma32Desc));
@@ -1758,6 +1759,7 @@ SdMmcCreateTrb (
   Trb->Packet    = Packet;
   Trb->Event     = Event;
   Trb->Started   = FALSE;
+  Trb->CommandComplete = FALSE;
   Trb->Timeout   = Packet->Timeout;
   Trb->Retries   = SD_MMC_TRB_RETRIES;
   Trb->Private   = Private;
@@ -2349,6 +2351,99 @@ SdMmcCheckAndRecoverErrors (
   return ErrorStatus;
 }
 
+/**
+  Reads the response data into the TRB buffer.
+  This function assumes that caller made sure that
+  command has completed.
+
+  @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  Response read successfully.
+  @retval Others       Failed to get response.
+**/
+EFI_STATUS
+SdMmcGetResponse (
+  IN SD_MMC_HC_PRIVATE_DATA  *Private,
+  IN SD_MMC_HC_TRB           *Trb
+  )
+{
+  EFI_SD_MMC_PASS_THRU_COMMAND_PACKET  *Packet;
+  UINT8                                Index;
+  UINT32                               Response[4];
+  EFI_STATUS                           Status;
+
+  Packet = Trb->Packet;
+
+  if (Packet->SdMmcCmdBlk->CommandType == SdMmcCommandTypeBc) {
+    return EFI_SUCCESS;
+  }
+
+  for (Index = 0; Index < 4; Index++) {
+    Status = SdMmcHcRwMmio (
+               Private->PciIo,
+               Trb->Slot,
+               SD_MMC_HC_RESPONSE + Index * 4,
+               TRUE,
+               sizeof (UINT32),
+               &Response[Index]
+               );
+      if (EFI_ERROR (Status)) {
+        return Status;
+      }
+    }
+  CopyMem (Packet->SdMmcStatusBlk, Response, sizeof (Response));
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Checks if the command completed. If the command
+  completed it gets the response and records the
+  command completion in the TRB.
+
+  @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   Command completed successfully.
+  @retval EFI_NOT_READY Command completion still pending.
+  @retval Others        Command failed to complete.
+**/
+EFI_STATUS
+SdMmcCheckCommandComplete (
+  IN SD_MMC_HC_PRIVATE_DATA  *Private,
+  IN SD_MMC_HC_TRB           *Trb,
+  IN UINT16                  IntStatus
+  )
+{
+  UINT16      Data16;
+  EFI_STATUS  Status;
+
+  if ((IntStatus & BIT0) != 0) {
+    Data16 = BIT0;
+    Status = SdMmcHcRwMmio (
+               Private->PciIo,
+               Trb->Slot,
+               SD_MMC_HC_NOR_INT_STS,
+               FALSE,
+               sizeof (Data16),
+               &Data16
+               );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    Status = SdMmcGetResponse (Private, Trb);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    Trb->CommandComplete = TRUE;
+    return EFI_SUCCESS;
+  }
+
+  return EFI_NOT_READY;
+}
+
 /**
   Check the TRB execution result.
 
@@ -2369,9 +2464,7 @@ SdMmcCheckTrbResult (
   EFI_STATUS                          Status;
   EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
   UINT16                              IntStatus;
-  UINT32                              Response[4];
   UINT64                              SdmaAddr;
-  UINT8                               Index;
   UINT32                              PioLength;
 
   Packet  = Trb->Packet;
@@ -2399,6 +2492,54 @@ SdMmcCheckTrbResult (
     goto Done;
   }
 
+  //
+  // Tuning commands are the only ones that do not generate command
+  // complete interrupt. Process them here before entering the code
+  // that waits for command completion.
+  //
+  if (((Private->Slot[Trb->Slot].CardType == EmmcCardType) &&
+       (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;
+    }
+  }
+
+  if (!Trb->CommandComplete) {
+    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.
   //
@@ -2456,65 +2597,9 @@ SdMmcCheckTrbResult (
     Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
   }
 
-  if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) &&
-      (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR1b) &&
-      (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR5b)) {
-    if ((IntStatus & BIT0) == BIT0) {
-      Status = EFI_SUCCESS;
-      goto Done;
-    }
-  }
-
-  if (((Private->Slot[Trb->Slot].CardType == EmmcCardType) &&
-       (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 = EFI_NOT_READY;
 Done:
-  //
-  // Get response data when the cmd is executed successfully.
-  //
-  if (!EFI_ERROR (Status)) {
-    if (Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeBc) {
-      for (Index = 0; Index < 4; Index++) {
-        Status = SdMmcHcRwMmio (
-                   Private->PciIo,
-                   Trb->Slot,
-                   SD_MMC_HC_RESPONSE + Index * 4,
-                   TRUE,
-                   sizeof (UINT32),
-                   &Response[Index]
-                   );
-        if (EFI_ERROR (Status)) {
-          SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
-          return Status;
-        }
-      }
-      CopyMem (Packet->SdMmcStatusBlk, Response, sizeof (Response));
-    }
-  }
 
   if (Status != EFI_NOT_READY) {
     SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
-- 
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.

[PATCHv2 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion

[Albecki, Mateusz]

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 38a8099426..5bdc200827 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -2444,6 +2444,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
+               );
+    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.
 
@@ -2464,7 +2570,6 @@ SdMmcCheckTrbResult (
   EFI_STATUS                          Status;
   EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
   UINT16                              IntStatus;
-  UINT64                              SdmaAddr;
   UINT32                              PioLength;
 
   Packet  = Trb->Packet;
@@ -2527,80 +2632,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.

[PATCHv2 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode

[Albecki, Mateusz]

Current driver does not support PIO transfer mode for
commands other then tuning. This change adds the code
to transfer PIO data.

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   | 132 +++++++++++++++++----
 2 files changed, 114 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 5bdc200827..73c43110f0 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: %ld\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: %p\n", Trb->Adma32Desc));
   DEBUG ((DebugLevel, "Adma64V3Desc: %p\n", Trb->Adma64V3Desc));
   DEBUG ((DebugLevel, "Adma64V4Desc: %p\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)) {
@@ -2444,6 +2448,104 @@ 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;
+  EFI_PCI_IO_PROTOCOL_WIDTH  Width;
+  UINTN                       Count;
+
+  BlockCount = (Trb->DataLen / Trb->BlockSize);
+  if (Trb->DataLen % Trb->BlockSize != 0) {
+    BlockCount += 1;
+  }
+
+  if (Trb->PioBlockIndex >= BlockCount) {
+    return EFI_SUCCESS;
+  }
+
+  switch (Trb->BlockSize % sizeof (UINT32)) {
+    case 0:
+      Width = EfiPciIoWidthFifoUint32;
+      Count = Trb->BlockSize / sizeof (UINT32);
+      break;
+    case 2:
+      Width = EfiPciIoWidthFifoUint16;
+      Count = Trb->BlockSize / sizeof (UINT16);
+      break;
+    case 1:
+    case 3:
+    default:
+      Width = EfiPciIoWidthFifoUint8;
+      Count = Trb->BlockSize;
+      break;
+    }
+
+  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,
+               Width,
+               Trb->Slot,
+               SD_MMC_HC_BUF_DAT_PORT,
+               Count,
+               (VOID*)((UINT8*)Trb->Data + (Trb->BlockSize * Trb->PioBlockIndex))
+               );
+    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,
+               Width,
+               Trb->Slot,
+               SD_MMC_HC_BUF_DAT_PORT,
+               Count,
+               (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.
 
@@ -2528,6 +2630,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 (
@@ -2570,7 +2679,6 @@ SdMmcCheckTrbResult (
   EFI_STATUS                          Status;
   EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
   UINT16                              IntStatus;
-  UINT32                              PioLength;
 
   Packet  = Trb->Packet;
   //
@@ -2606,26 +2714,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.

Re: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

[Wu, Hao A]

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, February 20, 2020 12:05 AM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao, Liming
> Subject: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command processing
> 
> This patch series aims to refactor command processing to achieve following
> 
> - Trace the failing TRB packets to see what commands are failing and for
> what reasons
> - Get the response data even if data transfer timed out to allow easier
> debugging
> - Fix the PIO mode which is currently completely broken.
> 
> Changes in v2:
> - Moved verbose packet prints after the command is finished to capture the
> successfull command response
> - Fixed the debug prints
> - PIO data will be moved with width matching the alignment of the block size.
> For majority of transfers that means UINT32 width.
> 
> Tests performed:
> - Each patch in the series has passed boot from eMMC with ADMAv3 data
> transfer mode
> - SDMA based boot has been tested with the full patch series
> - PIO based boot has been tested with the full patch series
> - PIO based data transfer has been additionally tested by creating and
> modyfing a file in EFI shell
> - Tested async PIO transfer - results below
> 
> Async test results:
> I've tested a simple async write and then readback from the eMMMC device
> using block IO v2. I have observed
> that while the requests are processed correctly by the eMMC driver as soon
> as they finish platform will sometimes
> mibehave. I've tested async without my changes and the strange behavior
> reproduces. Right now I am suspecting there
> is some problem either in the EDK2 core that performs async or in the
> platform specific code. It is also possible that
> I coded the Async BlockIo incorrectly although the test code was rather
> simple. Additionally I was able
> to observe that on many eMMC controllers PIO based data transfer is broken.
> I was only able to find one platform
> that supported PIO. Detailed observation below
> 
> Platform 1 (PIO working).
> - Test code was able to perform async write to eMMC LBA 0
> - As soon as the callback is called CPU exception happens
> - After reboot test code was able to perform async read
> - As soon as the callback is called CPU exception happens
> - After reboot sync read was able to confirm that data matches what was
> written by async write
> 
> Platform2 (PIO is returning trash data - all 0xAF)
> - Test code was able to perform async write(although it didn't realyl came
> through to the device)
> - After write finished test code was able to perform async read(again all data
> was 0xAF but the logic in the driver works)
> 
> Platform2 (again PIO is returning trash data)
> - Test code scheduled 5 async writes. 2 writes finished and after that CPU
> exception was signaled.
> 
> Platform3(also trash data from PIO)
> - Test code scheduled one async write as soon as it was done platform
> rebooted
> 
> I didn't want to spend any more time debugging this issue as I think it would
> turn into the platform debug and what I
> observed gave me some confidence that PIO in async is generally working.


Hello Mateusz,

Could you help to share your async test codes?
I can help to double confirm whether the issue you observed is related with
them or not.

Also, since edk2 repo is under soft freeze period for the next stable tag, I
would prefer for the series to get into the code base after the formal announce
of the stable tag (2020-02-28). I will still give the 'Reviewed-by' after my
review of the series though.

Do you have concern for this?

Best Regards,
Hao Wu


> 
> All tests were performed with eMMC in HS400 @200MHz clock frequency.
> 
> For easier review & integration patch has been pushed here:
> Whole series:
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor
> Whole series + SDMA force code(test 3):
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_sd
> ma
> Whole series + PIO force code(test 4):
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_pio
> 
> 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>
> 
> Mateusz Albecki (4):
>   MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
>   MdeModulePkg/SdMmcPciHcDxe: Read response on command completion
>   MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
>   MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode
> 
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 521
> ++++++++++++++++-----
>  2 files changed, 417 insertions(+), 108 deletions(-)
> 
> --
> 2.14.1.windows.1

Re: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

[Albecki, Mateusz]

Hi,

Github with test code: https://github.com/malbecki/edk2/tree/test_code_for_async

This test code was used as is on Platform2 and it worked if I have only scheduled one request. On platform 1 I had to rebuild it a couple of times to make it read instead of writing when testing after reboot.

Regarding the push - I am fine with this change making it to master after the stable tag.

Thanks,
Mateusz

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, February 20, 2020 1:40 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Marcin Wojtas <mw@semihalf.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command processing
> 
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Thursday, February 20, 2020 12:05 AM
> > To: devel@edk2.groups.io
> > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao,
> > Liming
> > Subject: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command
> > processing
> >
> > This patch series aims to refactor command processing to achieve
> > following
> >
> > - Trace the failing TRB packets to see what commands are failing and
> > for what reasons
> > - Get the response data even if data transfer timed out to allow
> > easier debugging
> > - Fix the PIO mode which is currently completely broken.
> >
> > Changes in v2:
> > - Moved verbose packet prints after the command is finished to capture
> > the successfull command response
> > - Fixed the debug prints
> > - PIO data will be moved with width matching the alignment of the block
> size.
> > For majority of transfers that means UINT32 width.
> >
> > Tests performed:
> > - Each patch in the series has passed boot from eMMC with ADMAv3 data
> > transfer mode
> > - SDMA based boot has been tested with the full patch series
> > - PIO based boot has been tested with the full patch series
> > - PIO based data transfer has been additionally tested by creating and
> > modyfing a file in EFI shell
> > - Tested async PIO transfer - results below
> >
> > Async test results:
> > I've tested a simple async write and then readback from the eMMMC
> > device using block IO v2. I have observed that while the requests are
> > processed correctly by the eMMC driver as soon as they finish platform
> > will sometimes mibehave. I've tested async without my changes and the
> > strange behavior reproduces. Right now I am suspecting there is some
> > problem either in the EDK2 core that performs async or in the platform
> > specific code. It is also possible that I coded the Async BlockIo
> > incorrectly although the test code was rather simple. Additionally I
> > was able to observe that on many eMMC controllers PIO based data
> > transfer is broken.
> > I was only able to find one platform
> > that supported PIO. Detailed observation below
> >
> > Platform 1 (PIO working).
> > - Test code was able to perform async write to eMMC LBA 0
> > - As soon as the callback is called CPU exception happens
> > - After reboot test code was able to perform async read
> > - As soon as the callback is called CPU exception happens
> > - After reboot sync read was able to confirm that data matches what
> > was written by async write
> >
> > Platform2 (PIO is returning trash data - all 0xAF)
> > - Test code was able to perform async write(although it didn't realyl
> > came through to the device)
> > - After write finished test code was able to perform async read(again
> > all data was 0xAF but the logic in the driver works)
> >
> > Platform2 (again PIO is returning trash data)
> > - Test code scheduled 5 async writes. 2 writes finished and after that
> > CPU exception was signaled.
> >
> > Platform3(also trash data from PIO)
> > - Test code scheduled one async write as soon as it was done platform
> > rebooted
> >
> > I didn't want to spend any more time debugging this issue as I think
> > it would turn into the platform debug and what I observed gave me some
> > confidence that PIO in async is generally working.
> 
> 
> Hello Mateusz,
> 
> Could you help to share your async test codes?
> I can help to double confirm whether the issue you observed is related with
> them or not.
> 
> Also, since edk2 repo is under soft freeze period for the next stable tag, I
> would prefer for the series to get into the code base after the formal
> announce of the stable tag (2020-02-28). I will still give the 'Reviewed-by'
> after my review of the series though.
> 
> Do you have concern for this?
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > All tests were performed with eMMC in HS400 @200MHz clock frequency.
> >
> > For easier review & integration patch has been pushed here:
> > Whole series:
> > https://github.com/malbecki/edk2/tree/emmc_transfer_refactor
> > Whole series + SDMA force code(test 3):
> >
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_sd
> > ma
> > Whole series + PIO force code(test 4):
> >
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_pio
> >
> > 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>
> >
> > Mateusz Albecki (4):
> >   MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
> >   MdeModulePkg/SdMmcPciHcDxe: Read response on command
> completion
> >   MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
> >   MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode
> >
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 521
> > ++++++++++++++++-----
> >  2 files changed, 417 insertions(+), 108 deletions(-)
> >
> > --
> > 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.

Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

[Albecki, Mateusz]

Hi,

After fixing the test app code(thanks Hao for reminding me that UEFI_APPLICATION is not the same as UEFI_DRIVER). I was able to execute 5 async writes and 5 async reads to eMMC device using PIO mode. Logs below:

Scheduling write 0
Scheduling write 1
Scheduling write 2
Scheduling write 3
Scheduling write 4
All writes scheduled
Emmc Async Request: CmdIndex[6] Arg[03B30000] Success
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[25] Arg[00000000] Success
Write Event done
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[25] Arg[00000001] Success
Write Event done
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[25] Arg[00000002] Success
Write Event done
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[25] Arg[00000003] Success
Write Event done
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[25] Arg[00000004] Success
Write Event done
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[18] Arg[00000000] Success
Dumping read buffer
 1 2 5 3 4 2 1 5 0 11
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[18] Arg[00000001] Success
Dumping read buffer
 1 2 5 3 4 2 1 5 0 11
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[18] Arg[00000002] Success
Dumping read buffer
 1 2 5 3 4 2 1 5 0 11
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[18] Arg[00000003] Success
Dumping read buffer
 1 2 5 3 4 2 1 5 0 11
Emmc Async Request: CmdIndex[23] Arg[00000001] Success
Emmc Async Request: CmdIndex[18] Arg[00000004] Success
Dumping read buffer
 1 2 5 3 4 2 1 5 0 11

Thanks,
Mateusz

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Albecki,
> Mateusz
> Sent: Thursday, February 20, 2020 1:36 PM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Marcin Wojtas <mw@semihalf.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe:
> Refactor command processing
> 
> Hi,
> 
> Github with test code:
> https://github.com/malbecki/edk2/tree/test_code_for_async
> 
> This test code was used as is on Platform2 and it worked if I have only
> scheduled one request. On platform 1 I had to rebuild it a couple of times to
> make it read instead of writing when testing after reboot.
> 
> Regarding the push - I am fine with this change making it to master after the
> stable tag.
> 
> Thanks,
> Mateusz
> 
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Thursday, February 20, 2020 1:40 AM
> > To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> > Cc: Marcin Wojtas <mw@semihalf.com>; Gao, Zhichao
> > <zhichao.gao@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
> > command processing
> >
> > > -----Original Message-----
> > > From: Albecki, Mateusz
> > > Sent: Thursday, February 20, 2020 12:05 AM
> > > To: devel@edk2.groups.io
> > > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao,
> > > Liming
> > > Subject: [PATCHv2 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor
> > command
> > > processing
> > >
> > > This patch series aims to refactor command processing to achieve
> > > following
> > >
> > > - Trace the failing TRB packets to see what commands are failing and
> > > for what reasons
> > > - Get the response data even if data transfer timed out to allow
> > > easier debugging
> > > - Fix the PIO mode which is currently completely broken.
> > >
> > > Changes in v2:
> > > - Moved verbose packet prints after the command is finished to
> > > capture the successfull command response
> > > - Fixed the debug prints
> > > - PIO data will be moved with width matching the alignment of the
> > > block
> > size.
> > > For majority of transfers that means UINT32 width.
> > >
> > > Tests performed:
> > > - Each patch in the series has passed boot from eMMC with ADMAv3
> > > data transfer mode
> > > - SDMA based boot has been tested with the full patch series
> > > - PIO based boot has been tested with the full patch series
> > > - PIO based data transfer has been additionally tested by creating
> > > and modyfing a file in EFI shell
> > > - Tested async PIO transfer - results below
> > >
> > > Async test results:
> > > I've tested a simple async write and then readback from the eMMMC
> > > device using block IO v2. I have observed that while the requests
> > > are processed correctly by the eMMC driver as soon as they finish
> > > platform will sometimes mibehave. I've tested async without my
> > > changes and the strange behavior reproduces. Right now I am
> > > suspecting there is some problem either in the EDK2 core that
> > > performs async or in the platform specific code. It is also possible
> > > that I coded the Async BlockIo incorrectly although the test code
> > > was rather simple. Additionally I was able to observe that on many
> > > eMMC controllers PIO based data transfer is broken.
> > > I was only able to find one platform that supported PIO. Detailed
> > > observation below
> > >
> > > Platform 1 (PIO working).
> > > - Test code was able to perform async write to eMMC LBA 0
> > > - As soon as the callback is called CPU exception happens
> > > - After reboot test code was able to perform async read
> > > - As soon as the callback is called CPU exception happens
> > > - After reboot sync read was able to confirm that data matches what
> > > was written by async write
> > >
> > > Platform2 (PIO is returning trash data - all 0xAF)
> > > - Test code was able to perform async write(although it didn't
> > > realyl came through to the device)
> > > - After write finished test code was able to perform async
> > > read(again all data was 0xAF but the logic in the driver works)
> > >
> > > Platform2 (again PIO is returning trash data)
> > > - Test code scheduled 5 async writes. 2 writes finished and after
> > > that CPU exception was signaled.
> > >
> > > Platform3(also trash data from PIO)
> > > - Test code scheduled one async write as soon as it was done
> > > platform rebooted
> > >
> > > I didn't want to spend any more time debugging this issue as I think
> > > it would turn into the platform debug and what I observed gave me
> > > some confidence that PIO in async is generally working.
> >
> >
> > Hello Mateusz,
> >
> > Could you help to share your async test codes?
> > I can help to double confirm whether the issue you observed is related
> > with them or not.
> >
> > Also, since edk2 repo is under soft freeze period for the next stable
> > tag, I would prefer for the series to get into the code base after the
> > formal announce of the stable tag (2020-02-28). I will still give the
> 'Reviewed-by'
> > after my review of the series though.
> >
> > Do you have concern for this?
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > All tests were performed with eMMC in HS400 @200MHz clock frequency.
> > >
> > > For easier review & integration patch has been pushed here:
> > > Whole series:
> > > https://github.com/malbecki/edk2/tree/emmc_transfer_refactor
> > > Whole series + SDMA force code(test 3):
> > >
> >
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_sd
> > > ma
> > > Whole series + PIO force code(test 4):
> > >
> >
> https://github.com/malbecki/edk2/tree/emmc_transfer_refactor_force_pio
> > >
> > > 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>
> > >
> > > Mateusz Albecki (4):
> > >   MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
> > >   MdeModulePkg/SdMmcPciHcDxe: Read response on command
> > completion
> > >   MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
> > >   MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode
> > >
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   4 +
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 521
> > > ++++++++++++++++-----
> > >  2 files changed, 417 insertions(+), 108 deletions(-)
> > >
> > > --
> > > 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.