* [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing
@ 2020-02-03 14:18 Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces Albecki, Mateusz
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-03 14:18 UTC (permalink / raw)
To: devel; +Cc: Mateusz Albecki, Hao A Wu, Marcin Wojtas, Zhichao Gao, Liming Gao
This patch series aims to refactor command processing to achieve following
1. Trace the failing TRB packets to see what commands are failing and for what reasons
2. Get the response data even if data transfer timed out to allow easier debugging
3. Fix the PIO mode which is currently completely broken.
Tests performed:
1. Each patch in the series has passed boot from eMMC with ADMAv3 data transfer mode
2. SDMA based boot has been tested with the full patch series
3. PIO based boot has been tested with the full patch series
4. PIO based data transfer has been additionally tested by creating and modyfing a file in EFI shell
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 | 502 ++++++++++++++++-----
2 files changed, 398 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
2020-02-03 14:18 [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Albecki, Mateusz
@ 2020-02-03 14:18 ` Albecki, Mateusz
2020-02-05 3:16 ` Wu, Hao A
2020-02-03 14:18 ` [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion Albecki, Mateusz
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-03 14:18 UTC (permalink / raw)
To: devel; +Cc: Mateusz Albecki, Hao A Wu, Marcin Wojtas, Zhichao Gao, Liming Gao
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..959645bf26 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: %d\n", Packet->Timeout));
+ DEBUG ((DebugLevel, "InDataBuffer: %X\n", Packet->InDataBuffer));
+ DEBUG ((DebugLevel, "OutDataBuffer: %X\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: %X\n", Trb->Data));
+ DEBUG ((DebugLevel, "DataLen: %d\n", Trb->DataLen));
+ DEBUG ((DebugLevel, "Read: %d\n", Trb->Read));
+ DEBUG ((DebugLevel, "DataPhy: %X\n", Trb->DataPhy));
+ DEBUG ((DebugLevel, "DataMap: %X\n", Trb->DataMap));
+ DEBUG ((DebugLevel, "Mode: %d\n", Trb->Mode));
+ DEBUG ((DebugLevel, "AdmaLengthMode: %d\n", Trb->AdmaLengthMode));
+ DEBUG ((DebugLevel, "Event: %d\n", Trb->Event));
+ DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
+ DEBUG ((DebugLevel, "Timeout: %d\n", Trb->Timeout));
+ DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
+ DEBUG ((DebugLevel, "Adma32Desc: %X\n", Trb->Adma32Desc));
+ DEBUG ((DebugLevel, "Adma64V3Desc: %X\n", Trb->Adma64V3Desc));
+ DEBUG ((DebugLevel, "Adma64V4Desc: %X\n", Trb->Adma64V4Desc));
+ DEBUG ((DebugLevel, "AdmaMap: %X\n", Trb->AdmaMap));
+ DEBUG ((DebugLevel, "AdmaPages: %X\n", Trb->AdmaPages));
+
+ SdMmcPrintPacket (DebugLevel, Trb->Packet);
+}
+
/**
Create a new TRB for the SD/MMC cmd request.
@@ -1963,6 +2039,9 @@ SdMmcExecTrb (
UINT64 AdmaAddr;
BOOLEAN AddressingMode64;
+ DEBUG ((DEBUG_VERBOSE, "Starting TRB execution\n"));
+ SdMmcPrintTrb (DEBUG_VERBOSE, Trb);
+
AddressingMode64 = FALSE;
Packet = Trb->Packet;
@@ -2235,6 +2314,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 +2521,10 @@ 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);
+ }
}
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.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion
2020-02-03 14:18 [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces Albecki, Mateusz
@ 2020-02-03 14:18 ` Albecki, Mateusz
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
2020-02-03 14:18 ` [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion Albecki, Mateusz
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-03 14:18 UTC (permalink / raw)
To: devel; +Cc: Mateusz Albecki, Hao A Wu, Marcin Wojtas, Zhichao Gao, Liming Gao
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 959645bf26..3dfaae8542 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: %d\n", Trb->Event));
DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
+ DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb->CommandComplete));
DEBUG ((DebugLevel, "Timeout: %d\n", Trb->Timeout));
DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
DEBUG ((DebugLevel, "Adma32Desc: %X\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;
@@ -2352,6 +2354,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.
@@ -2372,9 +2467,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;
@@ -2402,6 +2495,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.
//
@@ -2459,65 +2600,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.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
2020-02-03 14:18 [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion Albecki, Mateusz
@ 2020-02-03 14:18 ` Albecki, Mateusz
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
2020-02-03 14:18 ` [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode Albecki, Mateusz
2020-02-04 7:58 ` [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Wu, Hao A
4 siblings, 1 reply; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-03 14:18 UTC (permalink / raw)
To: devel; +Cc: Mateusz Albecki, Hao A Wu, Marcin Wojtas, Zhichao Gao, Liming Gao
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
+ );
+ 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.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode
2020-02-03 14:18 [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Albecki, Mateusz
` (2 preceding siblings ...)
2020-02-03 14:18 ` [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion Albecki, Mateusz
@ 2020-02-03 14:18 ` Albecki, Mateusz
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
2020-02-04 7:58 ` [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Wu, Hao A
4 siblings, 1 reply; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-03 14:18 UTC (permalink / raw)
To: devel; +Cc: Mateusz Albecki, Hao A Wu, Marcin Wojtas, Zhichao Gao, Liming Gao
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 | 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))
+ );
+ 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.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing
2020-02-03 14:18 [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Albecki, Mateusz
` (3 preceding siblings ...)
2020-02-03 14:18 ` [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode Albecki, Mateusz
@ 2020-02-04 7:58 ` Wu, Hao A
4 siblings, 0 replies; 13+ messages in thread
From: Wu, Hao A @ 2020-02-04 7:58 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io
Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
> -----Original Message-----
> From: 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: [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command
> processing
Hello Mateusz,
Please grant me some time for this series.
I do not have access to hardware in order to test this series at this moment,
will try to provide feedbacks early next week.
Best Regards,
Hao Wu
>
> This patch series aims to refactor command processing to achieve following
>
> 1. Trace the failing TRB packets to see what commands are failing and for
> what reasons
> 2. Get the response data even if data transfer timed out to allow easier
> debugging
> 3. Fix the PIO mode which is currently completely broken.
>
> Tests performed:
> 1. Each patch in the series has passed boot from eMMC with ADMAv3 data
> transfer mode
> 2. SDMA based boot has been tested with the full patch series
> 3. PIO based boot has been tested with the full patch series
> 4. PIO based data transfer has been additionally tested by creating and
> modyfing a file in EFI shell
>
> 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 | 502
> ++++++++++++++++-----
> 2 files changed, 398 insertions(+), 108 deletions(-)
>
> --
> 2.14.1.windows.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
2020-02-03 14:18 ` [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces Albecki, Mateusz
@ 2020-02-05 3:16 ` Wu, Hao A
0 siblings, 0 replies; 13+ messages in thread
From: Wu, Hao A @ 2020-02-05 3:16 UTC (permalink / raw)
To: Albecki, Mateusz, devel@edk2.groups.io
Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
Hello Mateusz,
Try to provide some feedbacks before I can test the patch.
Some inline comments below:
> -----Original Message-----
> From: 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: [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver
> traces
>
> 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..959645bf26 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: %d\n", Packet->Timeout));
Please update to use %ld for UINT64 here.
> + DEBUG ((DebugLevel, "InDataBuffer: %X\n", Packet->InDataBuffer));
> + DEBUG ((DebugLevel, "OutDataBuffer: %X\n", Packet->OutDataBuffer));
Please update to use %p for InDataBuffer/OutDataBuffer (VOID *).
> + 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: %X\n", Trb->Data));
Please update to use %p for Trb->Data (VOID *).
> + DEBUG ((DebugLevel, "DataLen: %d\n", Trb->DataLen));
> + DEBUG ((DebugLevel, "Read: %d\n", Trb->Read));
> + DEBUG ((DebugLevel, "DataPhy: %X\n", Trb->DataPhy));
Please update to use %lX for Trb->DataPhy (EFI_PHYSICAL_ADDRESS).
> + DEBUG ((DebugLevel, "DataMap: %X\n", Trb->DataMap));
Please update to use %p for Trb->DataMap (VOID *).
> + DEBUG ((DebugLevel, "Mode: %d\n", Trb->Mode));
> + DEBUG ((DebugLevel, "AdmaLengthMode: %d\n", Trb-
> >AdmaLengthMode));
> + DEBUG ((DebugLevel, "Event: %d\n", Trb->Event));
Please update to use %p for Trb->Event (EFI_EVENT).
> + DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
> + DEBUG ((DebugLevel, "Timeout: %d\n", Trb->Timeout));
Please update to use %ld for Trb->Timeout (UINT64).
> + DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
> + DEBUG ((DebugLevel, "Adma32Desc: %X\n", Trb->Adma32Desc));
> + DEBUG ((DebugLevel, "Adma64V3Desc: %X\n", Trb->Adma64V3Desc));
> + DEBUG ((DebugLevel, "Adma64V4Desc: %X\n", Trb->Adma64V4Desc));
> + DEBUG ((DebugLevel, "AdmaMap: %X\n", Trb->AdmaMap));
Please update to use %p for:
Trb->Adma32Desc
Trb->Adma64V3Desc
Trb->Adma64V4Desc
Trb->AdmaMap
(pointers)
Best Regards,
Hao Wu
> + DEBUG ((DebugLevel, "AdmaPages: %X\n", Trb->AdmaPages));
> +
> + SdMmcPrintPacket (DebugLevel, Trb->Packet);
> +}
> +
> /**
> Create a new TRB for the SD/MMC cmd request.
>
> @@ -1963,6 +2039,9 @@ SdMmcExecTrb (
> UINT64 AdmaAddr;
> BOOLEAN AddressingMode64;
>
> + DEBUG ((DEBUG_VERBOSE, "Starting TRB execution\n"));
> + SdMmcPrintTrb (DEBUG_VERBOSE, Trb);
> +
> AddressingMode64 = FALSE;
>
> Packet = Trb->Packet;
> @@ -2235,6 +2314,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 +2521,10 @@ 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);
> + }
> }
>
> return Status;
> --
> 2.14.1.windows.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion
2020-02-03 14:18 ` [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion Albecki, Mateusz
@ 2020-02-05 3:16 ` Wu, Hao A
2020-02-06 12:33 ` Albecki, Mateusz
0 siblings, 1 reply; 13+ messages in thread
From: Wu, Hao A @ 2020-02-05 3:16 UTC (permalink / raw)
To: devel@edk2.groups.io, Albecki, Mateusz
Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
One question 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 2/4] MdeModulePkg/SdMmcPciHcDxe: Read
> response on command completion
>
> 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 959645bf26..3dfaae8542 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: %d\n", Trb->Event));
> DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
> + DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb-
> >CommandComplete));
> DEBUG ((DebugLevel, "Timeout: %d\n", Trb->Timeout));
> DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
> DEBUG ((DebugLevel, "Adma32Desc: %X\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;
> @@ -2352,6 +2354,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
> + );
Previously, the driver cleans the Command Complete (BIT0) at the beginning of
the execution of the next TRB in function SdMmcExecTrb(). Now, the patch will
clean this bit just after checking it.
So the behavior in the patch is more aligned with the SD Host Controller Spec,
right?
Best Regards,
Hao Wu
> + 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.
>
> @@ -2372,9 +2467,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;
> @@ -2402,6 +2495,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.
> //
> @@ -2459,65 +2600,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.
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
2020-02-03 14:18 ` [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion Albecki, Mateusz
@ 2020-02-05 3:16 ` Wu, Hao A
2020-02-06 12:37 ` Albecki, Mateusz
0 siblings, 1 reply; 13+ messages in thread
From: Wu, Hao A @ 2020-02-05 3:16 UTC (permalink / raw)
To: devel@edk2.groups.io, Albecki, Mateusz
Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
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.
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode
2020-02-03 14:18 ` [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode Albecki, Mateusz
@ 2020-02-05 3:16 ` Wu, Hao A
2020-02-10 13:11 ` Albecki, Mateusz
0 siblings, 1 reply; 13+ messages in thread
From: Wu, Hao A @ 2020-02-05 3:16 UTC (permalink / raw)
To: devel@edk2.groups.io, Albecki, Mateusz
Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
> -----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.
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
@ 2020-02-06 12:33 ` Albecki, Mateusz
0 siblings, 0 replies; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-06 12:33 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io; +Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
Hi,
Yes, the new behavior should be more aligned with the SD host controller spec. I have been referring to section 3.7.1.2 The sequence to finalize a command which recommends to clear the interrupt in step 2 and then get the response data in step 3.
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 2/4] MdeModulePkg/SdMmcPciHcDxe:
> Read response on command completion
>
> One question 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 2/4] MdeModulePkg/SdMmcPciHcDxe: Read
> > response on command completion
> >
> > 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 959645bf26..3dfaae8542 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: %d\n", Trb->Event));
> > DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
> > + DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb-
> > >CommandComplete));
> > DEBUG ((DebugLevel, "Timeout: %d\n", Trb->Timeout));
> > DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
> > DEBUG ((DebugLevel, "Adma32Desc: %X\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;
> > @@ -2352,6 +2354,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
> > + );
>
>
> Previously, the driver cleans the Command Complete (BIT0) at the beginning
> of the execution of the next TRB in function SdMmcExecTrb(). Now, the
> patch will clean this bit just after checking it.
>
> So the behavior in the patch is more aligned with the SD Host Controller Spec,
> right?
>
> Best Regards,
> Hao Wu
>
>
> > + 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.
> >
> > @@ -2372,9 +2467,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;
> > @@ -2402,6 +2495,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.
> > //
> > @@ -2459,65 +2600,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.
> >
> >
> >
>
--------------------------------------------------------------------
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] 13+ messages in thread
* Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
@ 2020-02-06 12:37 ` Albecki, Mateusz
0 siblings, 0 replies; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-06 12:37 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io; +Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
Hi,
As in the patch 2/4 section 3.7.1.2 recommends that we clear the transfer complete(step 6) just after reading it(step 5).
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 3/4] MdeModulePkg/SdMmcPciHcDxe:
> Refactor data transfer completion
>
> 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.
> >
> >
> >
>
--------------------------------------------------------------------
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] 13+ messages in thread
* Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
@ 2020-02-10 13:11 ` Albecki, Mateusz
0 siblings, 0 replies; 13+ messages in thread
From: Albecki, Mateusz @ 2020-02-10 13:11 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io; +Cc: Marcin Wojtas, Gao, Zhichao, Gao, Liming
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-02-10 13:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-03 14:18 [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces Albecki, Mateusz
2020-02-05 3:16 ` Wu, Hao A
2020-02-03 14:18 ` [PATCH 2/4] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion Albecki, Mateusz
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
2020-02-06 12:33 ` Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 3/4] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion Albecki, Mateusz
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
2020-02-06 12:37 ` Albecki, Mateusz
2020-02-03 14:18 ` [PATCH 4/4] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode Albecki, Mateusz
2020-02-05 3:16 ` [edk2-devel] " Wu, Hao A
2020-02-10 13:11 ` Albecki, Mateusz
2020-02-04 7:58 ` [PATCH 0/4] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing 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