Re: [PATCH 1/4] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces

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

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