Re: [Linaro-uefi] [PATCH 2/4] Drivers/SdMmc: Adjust SdMmcPciHcDxe to fulfill Xenon controller demands

17 Nov 2016

Hi Leif,
Thanks a lot for your remarks. Please find comments inline.
Best Regards,
Jan
...
...

//
// Get SD/MMC Pci Host Controller Slot info
//
Status = SdMmcHcGetSlotInfo (PciIo, &FirstBar, &SlotNum);
if (EFI_ERROR (Status)) {
goto Done;
}
Support64BitDma = TRUE;
for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {


for (Slot = 0; Slot < (XENON_MMC_SLOT_ID_HYPERION + 1); Slot++) {

Since all this line is currently doing is hardcoding that this loop
should only be executed once, could an intermediate define of
..._NUM_SLOTS be introduced, if we're expecting this driver to be
reusable for anything else? (And if we don't, can we drop the loop
bit?)
Indeed, current solution isn't reusable. We should drop the loop.
...
...

//
// Override capabilities structure - only 4 Bit width bus is supported
// by HW and also force using SDR25 mode
//
Private->Capability[Slot].Sdr104 = 0;
Private->Capability[Slot].Ddr50 = 0;
Private->Capability[Slot].Sdr50 = 0;
Private->Capability[Slot].BusWidth8 = 0;

And if you are importing the whole driver, I'm guessing you can get
rid of this type of thing completely?
We want to have as small diff as possible in comparison to
MdeModulePkg stack. If we wouldn't do this here, few other steps in
init sequence will have to be changed. Overriding capability fields in
one place (which allows to clearly see what are differences between HW
capabilities and software support) is cleaner way in my opinion. What
do you think?
...
...


if (Private->Capability[Slot].BaseClkFreq == 0) {
 Private->Capability[Slot].BaseClkFreq = 0xff;


}

DumpCapabilityReg (Slot, &Private->Capability[Slot]);
Status = SdMmcHcGetMaxCurrent (PciIo, Slot, &Private->MaxCurrent[Slot]);
   if (EFI_ERROR (Status)) {
     continue;

@@ -624,28 +629,8 @@ SdMmcPciHcDriverBindingStart (
       continue;
     }

//
// Reset the specified slot of the SD/MMC Pci Host Controller
//
Status = SdMmcHcReset (PciIo, Slot);
if (EFI_ERROR (Status)) {
 continue;


}
//
// Check whether there is a SD/MMC card attached
//
Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
 continue;


} else if (!MediaPresent) {
 DEBUG ((DEBUG_ERROR, "SdMmcHcCardDetect: No device attached in Slot[%d]!!!\n", Slot));


 continue;


}

Status = SdMmcHcInitHost (PciIo, Slot, Private->Capability[Slot]);
if (EFI_ERROR (Status)) {
 continue;


}


// Perform Xenon-specific init sequence

XenonInit (Private);
Private->Slot[Slot].MediaPresent = TRUE;
Private->Slot[Slot].Initialized  = TRUE;


@@ -889,6 +874,220 @@ SdMmcPciHcDriverBindingStop (
 }
/**

Check if card is ready for next data transfer by reading its status.

@param[in]  This              A pointer to the EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
@param[in]  Slot              The slot number of the device to send the command to.
@param[in]  Rca               The relative device address of addressed device.
@param[in]  Timeout           The timeout in miliseconds.

@retval EFI_SUCCESS           Card is ready for next data transfer.
@retval EFI_DEVICE_ERROR      Card status is erroneous.
@retval EFI_TIMEOUT           Card is busy.


+**/
+STATIC
+EFI_STATUS
+SdMmcIsReady (

IN EFI_SD_MMC_PASS_THRU_PROTOCOL *This,
IN UINT8                         Slot,
IN UINT16                        Rca,
IN UINTN                         Timeout
)

+{

EFI_STATUS Status;
UINT32 DevStatus;

do {
Status = SdCardSendStatus (This, Slot, Rca, &DevStatus);
if (EFI_ERROR (Status)) {
 DEBUG ((DEBUG_ERROR, "Cannot read SD status\n"));


 return Status;


}

// Check device status
if ((DevStatus & (1 << 8)) && (DevStatus & (0xf << 9)) != (7 << 9)) {

Some macros here to show which flags are being checked, please?
Yes, whole function should be corrected in terms of magic numbers.
...
...

 break;


} else if (DevStatus & ~0x0206BF7F) {

More macros, please.
...

 DEBUG ((DEBUG_ERROR, "SD Status error\n"));


 return EFI_DEVICE_ERROR;


}

gBS->Stall (1000);

And a comment with that delay please. (Why? Why this value?)
Ok.
...
...

} while (Timeout--);

if (Timeout <= 0) {
DEBUG ((DEBUG_ERROR, "SD Status timeout\n"));
return EFI_TIMEOUT;
}

if (DevStatus & (1 << 7)) {

Macro?
Ok
...
...

DEBUG ((DEBUG_ERROR, "SD switch error\n"));
return EFI_DEVICE_ERROR;
}

return EFI_SUCCESS;

+}



+STATIC
+EFI_STATUS
+CheckParameters (

IN EFI_SD_MMC_PASS_THRU_PROTOCOL         *This,
IN EFI_SD_MMC_PASS_THRU_COMMAND_PACKET   *Packet,
IN UINT8                                 Slot
)

+{

SD_MMC_HC_PRIVATE_DATA *Private;

Private = SD_MMC_HC_PRIVATE_FROM_THIS (This);

if ((This == NULL) || (Packet == NULL)) {
return EFI_INVALID_PARAMETER;
}

if ((Packet->SdMmcCmdBlk == NULL) || (Packet->SdMmcStatusBlk == NULL)) {
return EFI_INVALID_PARAMETER;
}

if ((Packet->OutDataBuffer == NULL) && (Packet->OutTransferLength != 0)) {
return EFI_INVALID_PARAMETER;
}

if ((Packet->InDataBuffer == NULL) && (Packet->InTransferLength != 0)) {
return EFI_INVALID_PARAMETER;
}

if (!Private->Slot[Slot].Enable) {
return EFI_INVALID_PARAMETER;
}

if (!Private->Slot[Slot].MediaPresent) {
return EFI_NO_MEDIA;
}

// Currently we don't support SDIO
if (Packet->SdMmcCmdBlk->CommandIndex == SDIO_SEND_OP_COND) {
return EFI_DEVICE_ERROR;
}

return EFI_SUCCESS;

+}



+/**

Send command SD_STOP_TRANSMISSION to stop multiple block transfer.

@param[in]  This              A pointer to the EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
@param[in]  Slot              The slot number of the device to send the command to.

@retval EFI_SUCCESS           The request is executed successfully.
@retval Others                The request could not be executed successfully.


+**/
+STATIC
+EFI_STATUS
+SdMmcSendStopTransmission (

IN     EFI_SD_MMC_PASS_THRU_PROTOCOL *This,
IN     UINT8                         Slot
)

+{

EFI_STATUS                           Status;
EFI_SD_MMC_COMMAND_BLOCK             SdMmcCmdBlk;
EFI_SD_MMC_STATUS_BLOCK              SdMmcStatusBlk;
EFI_SD_MMC_PASS_THRU_COMMAND_PACKET  Packet;

ZeroMem (&SdMmcCmdBlk, sizeof (SdMmcCmdBlk));
ZeroMem (&SdMmcStatusBlk, sizeof (SdMmcStatusBlk));
ZeroMem (&Packet, sizeof (Packet));

Packet.SdMmcCmdBlk    = &SdMmcCmdBlk;
Packet.SdMmcStatusBlk = &SdMmcStatusBlk;
Packet.Timeout        = 2500 * 1000; // SD generic timeout

Sure, but can we have some defines for it please?
Ok, will replace comment with define.
...
...


SdMmcCmdBlk.CommandIndex = SD_STOP_TRANSMISSION;
SdMmcCmdBlk.CommandType  = SdMmcCommandTypeAc;
SdMmcCmdBlk.ResponseType = SdMmcResponseTypeR1b;
SdMmcCmdBlk.CommandArgument = 0;

Status = This->PassThru (This, Slot, &Packet, NULL);

return Status;

+}



+STATIC
+EFI_STATUS
+WaitForInhibit (

IN SD_MMC_HC_PRIVATE_DATA              *Private,
IN EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet,
IN UINT8                               Slot,
IN BOOLEAN                             DataTransfer
)

+{

UINT32 Mask, PresentState, Time = 0;
UINT32 CmdTimeout = XENON_MMC_CMD_DEFAULT_TIMEOUT;

if (Packet->SdMmcCmdBlk != NULL) {
Mask = BIT0; // SDHCI_CMD_INHIBIT
}

if (DataTransfer != FALSE) {
Mask = BIT1; // SDHCI_DATA_INHIBIT
}

//
// We shouldn't wait for data inhibit for stop commands, even
// though they might use busy signaling
//
if (Packet->SdMmcCmdBlk->CommandIndex == SD_STOP_TRANSMISSION)
Mask &= ~BIT1;

SdMmcHcRwMmio (
     Private->PciIo,


     Slot,


     SD_MMC_HC_PRESENT_STATE,


     TRUE,


     sizeof (PresentState),


     &PresentState


   );



while (PresentState & Mask) {
if (Time >= CmdTimeout) {
 if (2 * CmdTimeout <= XENON_MMC_CMD_MAX_TIMEOUT) {


   CmdTimeout += CmdTimeout;


 } else {


   if (Packet->SdMmcCmdBlk != NULL) {


     XenonReset (Private, Slot, 0x02); // CMD reset



Could replace the comment with a #define?
Ok.
...
...

   }



   if (DataTransfer) {


     XenonReset (Private, Slot, 0x04); // DATA reset



Could replace the comment with a #define?
Ok.
...
...

   }



   DEBUG((DEBUG_ERROR, "MvSdMmcPassThru: Timeout...\n"));



   return EFI_TIMEOUT;


 }


}

Time++;
gBS->Stall (1000);

SdMmcHcRwMmio (
       Private->PciIo,


       Slot,


       SD_MMC_HC_PRESENT_STATE,


       TRUE,


       sizeof (PresentState),


       &PresentState


     );


}

return EFI_SUCCESS;

+}



+// Variable to store current card's Rca - used in polling card status
+STATIC UINT16 CurrRca = 0;
I think this should have an m-prefix.
Agree.
...
...



+/**
   Sends SD command to an SD card that is attached to the SD controller.
The PassThru() function sends the SD command specified by Packet to the SD card
@@ -934,86 +1133,351 @@ SdMmcPassThruPassThru (
   IN     EFI_EVENT                             Event    OPTIONAL
   )
 {

EFI_STATUS                      Status;
SD_MMC_HC_PRIVATE_DATA          *Private;
SD_MMC_HC_TRB                   *Trb;
EFI_TPL                         OldTpl;


EFI_STATUS Status;
SD_MMC_HC_PRIVATE_DATA *Private;
INTN Ret;
UINTN *Data, DataLen, Index;
UINT32 Argument, Response[4], Retry, Stat, Mask;

Please not Stat and Status in the same function.
Ok, will change first one.
...
...

UINT16 BlockSize, BlkCount;
UINT16 Cmd, IntStatus, TimeoutCtrl, Mode, BlkSizeReg;
BOOLEAN MultiFlag = FALSE, SetRcaFlag = FALSE, Read = FALSE;
BOOLEAN DataTransfer;


Data = NULL;
DataLen = 0;
Retry = 1000;

Could we have a define for that?
Ok.
...
...

Stat = 0;
Ret = 0;
BlockSize = 0x200;

SIZE_512B?
(We don't have a global define for that, but it would make it more readable.)
Ok.
...
...

BlkCount = 0;

Status = CheckParameters (This, Packet, Slot);
if (EFI_ERROR(Status)) {
return Status;
}


if ((This == NULL) || (Packet == NULL)) {
return EFI_INVALID_PARAMETER;


Private = SD_MMC_HC_PRIVATE_FROM_THIS (This);

//
// Mark if this is multiblock data transfer since some special
// acctions must be taken in such circumstances, i.e. it's necessary
// to send SD_STOP_TRANSMISSION command.
//
if (Packet->SdMmcCmdBlk->CommandIndex == SD_WRITE_MULTIPLE_BLOCK ||
 Packet->SdMmcCmdBlk->CommandIndex == SD_READ_MULTIPLE_BLOCK) {


MultiFlag = TRUE;
}


if ((Packet->SdMmcCmdBlk == NULL) || (Packet->SdMmcStatusBlk == NULL)) {
return EFI_INVALID_PARAMETER;


//
// Mark if this is command to set new relative card address (RCA),
// since it's necessary to have updated copy of current RCA in this
// layer - used in checking card status.
//
if (Packet->SdMmcCmdBlk->CommandIndex == SD_SET_RELATIVE_ADDR) {
SetRcaFlag = TRUE;
}


if ((Packet->OutDataBuffer == NULL) && (Packet->OutTransferLength != 0)) {
return EFI_INVALID_PARAMETER;


// Prepare data transfer's flags
if (((Packet->InTransferLength != 0) && (Packet->InDataBuffer != NULL)) ||
 ((Packet->OutTransferLength != 0) && (Packet->OutDataBuffer != NULL))) {


DataTransfer = FALSE;
}


if ((Packet->InDataBuffer == NULL) && (Packet->InTransferLength != 0)) {
return EFI_INVALID_PARAMETER;


if ((Packet->InTransferLength != 0) && (Packet->InDataBuffer != NULL)) {
DataLen = Packet->InTransferLength;
Data = Packet->InDataBuffer;
Read = TRUE;
DataTransfer = TRUE;
}


Private = SD_MMC_HC_PRIVATE_FROM_THIS (This);


if ((Packet->OutTransferLength != 0) && (Packet->OutDataBuffer != NULL)) {
DataLen = Packet->OutTransferLength;
Data = Packet->OutDataBuffer;
DataTransfer = TRUE;
}


if (!Private->Slot[Slot].Enable) {
return EFI_INVALID_PARAMETER;


// Clear ERROR_IRQ status
IntStatus = 0xFFFF;
Status = SdMmcHcRwMmio (
              Private->PciIo,


              Slot,


              SD_MMC_HC_ERR_INT_STS,


              FALSE,


              sizeof (IntStatus),


              &IntStatus


            );



if (EFI_ERROR (Status)) {
DEBUG((DEBUG_ERROR, "MvSdMmcPassThru: Cannot clear ERROR_IRQ status\n"));
return Status;
}


if (!Private->Slot[Slot].MediaPresent) {
return EFI_NO_MEDIA;


// Clear IRQ status
Status = SdMmcHcRwMmio (
              Private->PciIo,


              Slot,


              SD_MMC_HC_NOR_INT_STS,


              FALSE,


              sizeof (IntStatus),


              &IntStatus


            );



if (EFI_ERROR (Status)) {
DEBUG((DEBUG_ERROR, "MvSdMmcPassThru: Cannot clear IRQ status\n"));
return Status;
}


if (!Private->Slot[Slot].Initialized) {
return EFI_DEVICE_ERROR;


WaitForInhibit (Private, Packet, Slot, DataTransfer);

Cmd = (UINT16)LShiftU64(Packet->SdMmcCmdBlk->CommandIndex, 8);
if (Packet->SdMmcCmdBlk->CommandType == SdMmcCommandTypeAdtc) {
Cmd |= BIT5;

Is it possible to give this bit a more descriprive name?
Please see below comment.
...
...
}

Trb = SdMmcCreateTrb (Private, Slot, Packet, Event);
if (Trb == NULL) {
return EFI_OUT_OF_RESOURCES;


if (Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeBc) {
switch (Packet->SdMmcCmdBlk->ResponseType) {
 case SdMmcResponseTypeR1:


 case SdMmcResponseTypeR5:


 case SdMmcResponseTypeR6:


 case SdMmcResponseTypeR7:


   Cmd |= (BIT1 | BIT3 | BIT4);



Is it possible to give these bits more descriprive names?
If not, please sort them in descending order.
(Same applies to rest of cases.)
This switch-case statement is taken from MdeModulePkg driver and
ResponsesTypes (with according BITs in CMD) are well documented in SD
specification. But if you think it isn't enough, I will add #defines.
...
...

   break;


 case SdMmcResponseTypeR2:


   Cmd |= (BIT0 | BIT3);


  break;


 case SdMmcResponseTypeR3:


 case SdMmcResponseTypeR4:


   Cmd |= BIT1;


   break;


 case SdMmcResponseTypeR1b:


 case SdMmcResponseTypeR5b:


   Cmd |= (BIT0 | BIT1 | BIT3 | BIT4);


   break;


 default:


   ASSERT (FALSE);



Should this not return also?
In a release build, this would just keep executing in an unknown state.
Yes, agree.
...
...

   break;


}
}


//
// Immediately return for async I/O.
//
if (Event != NULL) {
return EFI_SUCCESS;



if ((DataLen % BlockSize) != 0) {
if (DataLen < BlockSize) {
 BlockSize = (UINT16)DataLen;


}
}
BlkCount = (UINT16) (DataLen / BlockSize);

if (DataTransfer) {

// Set default timeout value
TimeoutCtrl = 0xE;

#define?
Ok.
...
...

SdMmcHcRwMmio (
        Private->PciIo,


        Slot,


        SD_MMC_HC_TIMEOUT_CTRL,


        FALSE,


        sizeof (TimeoutCtrl),


        &TimeoutCtrl


      );



Mode = 0x02;    // SDHCI_TRNS_BLK_CNT_EN
if (BlkCount > 1) {
 Mode |= 0x20; // SDHCI_TRNS_MULTI


}
if (Read) {
 Mode |= 0x10; // SDHCI_TRNS_READ



#defines instead of comments?
Ok.
...
...

}


//
// Wait async I/O list is empty before execute sync I/O operation.
//
while (TRUE) {
OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
if (IsListEmpty (&Private->Queue)) {
 gBS->RestoreTPL (OldTpl);




// Fill BlockSize register
BlkSizeReg = (0x7 << 12) | (BlockSize & 0xFFF);

#defines?
(SIZE_4KB - 1)?
Ok.
...
...

SdMmcHcRwMmio (
        Private->PciIo,


        Slot,


        SD_MMC_HC_BLK_SIZE,


        FALSE,


        sizeof (BlkSizeReg),


        &BlkSizeReg


      );



SdMmcHcRwMmio (
        Private->PciIo,


        Slot,


        SD_MMC_HC_BLK_COUNT,


        FALSE,


        sizeof (BlkCount),


        &BlkCount


      );



SdMmcHcRwMmio (
        Private->PciIo,


        Slot,


        SD_MMC_HC_TRANS_MOD,


        FALSE,


        sizeof (Mode),


        &Mode


      );


}

Argument = Packet->SdMmcCmdBlk->CommandArgument;
SdMmcHcRwMmio (
      Private->PciIo,


      Slot,


      SD_MMC_HC_ARG1,


      FALSE,


      sizeof (Argument),


      &Argument


    );



// Execute cmd
SdMmcHcRwMmio (
      Private->PciIo,


      Slot,


      SD_MMC_HC_COMMAND,


      FALSE,


      sizeof (Cmd),


      &Cmd


    );



Mask = BIT0; // SDHCI_INT_RESPONSE

#define instead of comment?
Ok. Will apply to every comment of this type.
...
...

if (Packet->SdMmcCmdBlk->ResponseType == SdMmcResponseTypeR1b ||
 Packet->SdMmcCmdBlk->ResponseType == SdMmcResponseTypeR5b) {


Mask |= BIT1;

#define instead of comment?
...

}

do {
Status = SdMmcHcRwMmio (
                Private->PciIo,


                Slot,


                SD_MMC_HC_NOR_INT_STS,


                TRUE,


                sizeof (IntStatus),


                &IntStatus


              );



if (EFI_ERROR (Status)) {
 return EFI_INVALID_PARAMETER;


}

if (IntStatus & BIT15) { // SDHCI_INT_STATUS

#define instead of comment?
...

 DEBUG((DEBUG_INFO, "MvSdMmcPassThru: SDHCI_INT_ERROR stat:%x\n",


   IntStatus));
 break;

}


gBS->RestoreTPL (OldTpl);

}

Status = SdMmcWaitTrbEnv (Private, Trb);

if (EFI_ERROR (Status)) {

goto Done;



// Stall for 100 microseconds

Yeah, that's what the line below says on its own. Why? Why this period?
Ok, we will add more precise description to every delay.
...
...

gBS->Stall (100);
if (--Retry == 0) {
 break;


}
} while ((IntStatus & Mask) != Mask);

if (Retry == 0) {
DEBUG((DEBUG_ERROR, "MvSdMmcPassThru: Rx timeout\n"));
return EFI_TIMEOUT;
}


Status = SdMmcExecTrb (Private, Trb);
if (EFI_ERROR (Status)) {
goto Done;


if ((IntStatus & (BIT15 | Mask)) == Mask) {

#define?
Ok.
...
...

if (Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeBc) {
 for (Index = 0; Index < 4; Index++) {


   SdMmcHcRwMmio (


           Private->PciIo,


           Slot,


           SD_MMC_HC_RESPONSE + Index * 4,


           TRUE,


           sizeof (UINT32),



sizeof (Response[Index])? (Or sizeof (*Response)?)
Yes, first option will suit here.
...
...

           &Response[Index]


         );


 }



 CopyMem (Packet->SdMmcStatusBlk, Response, sizeof (Response));



 //


 // Update RCA of current card in order to use it later


 // for polling card status


 //


 if (SetRcaFlag)



Always {} with if.
Ok.
...
...

   CurrRca = Packet->SdMmcStatusBlk->Resp0 >> 16;



#define?
As with my comment to switch-case statement. Such shift for RCA is
commonly used in MdeModulePkg, but for clarity we will make it
#define.
...
...

}

IntStatus = Mask;
Status = SdMmcHcRwMmio (
                Private->PciIo,


                Slot,


                SD_MMC_HC_NOR_INT_STS,


                FALSE,


                sizeof (IntStatus),


                &IntStatus


              );



} else {
Ret = -1;
}


Status = SdMmcWaitTrbResult (Private, Trb);
if (EFI_ERROR (Status)) {
goto Done;


if (!Ret && DataTransfer) {
Status = XenonTransferData (Private, Slot, Data, DataLen, BlockSize, BlkCount, Read);
}

-Done:

if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
FreePages (Trb->AdmaDesc, Trb->AdmaPages);


// SDHCI_QUIRK_WAIT_SEND_CMD - 1 milisecond delay

The code already says it's a 1 milisecond delay. Why is there a delay,
and why is it of this period?
Will add proper comment.
...
...

gBS->Stall (1000);

// Save current IRQ status
Status = SdMmcHcRwMmio (
              Private->PciIo,


              Slot,


              SD_MMC_HC_NOR_INT_STS,


              TRUE,


              sizeof (IntStatus),


              &IntStatus


            );



// Clear IRQ
Stat = 0xFFFFFFFFULL;

Stat (hopefully to be renamed) is UINT32, so why ULL?
But would also look much better with a #define.
ULL sufix isn't necessary here. Ok.
...
...

Status = SdMmcHcRwMmio (
              Private->PciIo,


              Slot,


              SD_MMC_HC_NOR_INT_STS,


              FALSE,


              sizeof (Stat),


              &Stat


            );



//
// There are two quirks for DataTransfer commands:
// 1. For multipleblock write/read it's necessary to issue
// SD_STOP_TRANSMISSION command
// 2. For every write data comand, it's necessary to poll for
// card status before issuing next data transfer
//
if (!Ret && DataTransfer) {
if (MultiFlag) {
 SdMmcSendStopTransmission(This, Slot);


}

if (!Read) {
 return SdMmcIsReady (This, Slot, CurrRca, 1000);



What is 1000?
It's max timeout value for which we will poll device status. Will add
proper define to make it more clear.
...
...

}

return EFI_SUCCESS;
}


if (Trb != NULL) {
FreePool (Trb);


// Check previously saved IRQ status for non-data commands
if (IntStatus & BIT15) {
if (Packet->SdMmcCmdBlk != NULL) {
 XenonReset (Private, Slot, 0x02); // CMD



#define instead of comment.
Ok, will apply for whole file.
...
...

}

if (DataTransfer) {
 XenonReset (Private, Slot, 0x04); // DATA



#define instead of comment.
...

}

if (IntStatus & (0x10000 | 0x100000)) { // SDHCI_INT_TIMEOUT

#defines.
...

 DEBUG((DEBUG_ERROR, "MvSdMmcPassThru: Timeout!\n"));


 return EFI_TIMEOUT;


} else {

 return EFI_DEVICE_ERROR;


}

} else {

return EFI_SUCCESS;
}
return Status;

diff --git a/Drivers/SdMmc/MvSdMmcPciHcDxe.inf b/Drivers/SdMmc/MvSdMmcPciHcDxe.inf
new file mode 100644
index 0000000..a4f66e9
--- /dev/null
+++ b/Drivers/SdMmc/MvSdMmcPciHcDxe.inf
@@ -0,0 +1,75 @@
+#/*******************************************************************************
+#Copyright (C) 2016 Marvell International Ltd.
+#
+#Marvell BSD License Option
+#
+#If you received this File from Marvell, you may opt to use, redistribute and/or
+#modify this File under the following licensing terms.
+#Redistribution and use in source and binary forms, with or without modification,
+#are permitted provided that the following conditions are met:
+#
+#* Redistributions of source code must retain the above copyright notice,
+#  this list of conditions and the following disclaimer.
+#
+#* Redistributions in binary form must reproduce the above copyright
+#  notice, this list of conditions and the following disclaimer in the
+#  documentation and/or other materials provided with the distribution.
+#
+#* Neither the name of Marvell nor the names of its contributors may be
+#  used to endorse or promote products derived from this software without
+#  specific prior written permission.
+#
+#THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+#ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+#WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+#DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+#ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+#(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+#LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+#ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+#SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#
+#*******************************************************************************/



+[Defines]

INF_VERSION                    = 0x00010005

0019
Ok.
...
...

BASE_NAME                      = MvSdMmcPciHcDxe
MODULE_UNI_FILE                = ../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.uni

Refer to local copies.
Ok.
...
...

FILE_GUID                      = 5b5b83b8-52b8-4d02-b92b-c84a4dfb539d
MODULE_TYPE                    = UEFI_DRIVER
VERSION_STRING                 = 1.0
ENTRY_POINT                    = InitializeSdMmcPciHcDxe


+[Sources]

../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/ComponentName.c
../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h

Refer to local copies.
Ok.
...
...

MvSdMmcPciHcDxe.c
XenonSdhci.c
XenonSdhci.h


+[Packages]

MdePkg/MdePkg.dec


+[LibraryClasses]

BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
MemoryAllocationLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
UefiRuntimeServicesTableLib


+[Protocols]

gEfiDevicePathProtocolGuid                    ## TO_START
gEfiPciIoProtocolGuid                         ## TO_START
gEfiSdMmcPassThruProtocolGuid                 ## BY_START


+[UserExtensions.TianoCore."ExtraFiles"]

SdMmcPciHcDxeExtra.uni

diff --git a/Drivers/SdMmc/XenonSdhci.c b/Drivers/SdMmc/XenonSdhci.c
new file mode 100755
index 0000000..6a20105
--- /dev/null
+++ b/Drivers/SdMmc/XenonSdhci.c
@@ -0,0 +1,661 @@
+/*******************************************************************************
+Copyright (C) 2016 Marvell International Ltd.



+Marvell BSD License Option



+If you received this File from Marvell, you may opt to use, redistribute and/or
+modify this File under the following licensing terms.
+Redistribution and use in source and binary forms, with or without modification,
+are permitted provided that the following conditions are met:



+* Redistributions of source code must retain the above copyright notice,

this list of conditions and the following disclaimer.


+* Redistributions in binary form must reproduce the above copyright

notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.


+* Neither the name of Marvell nor the names of its contributors may be

used to endorse or promote products derived from this software without
specific prior written permission.


+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.



+*******************************************************************************/



+#include "XenonSdhci.h"



+STATIC
+VOID
+XenonSetMpp (

VOID
)

+{

UINT32 Reg;

// Set eMMC/SD PHY output instead of MPPs

As pin control configuration, isn't this something that should be done
in Armada70x0Lib or similar?
This code will be moved in next spin.
...
...

Reg = MmioRead32 (MVEBU_IP_CONFIG_REG);
Reg &= ~(1 << 0);

#define
Ok.
...
...

MmioWrite32 (MVEBU_IP_CONFIG_REG, Reg);

+}



+STATIC
+VOID
+XenonReadVersion (

IN  EFI_PCI_IO_PROTOCOL   *PciIo,
OUT UINT32 *ControllerVersion
)

+{


SdMmcHcRwMmio (PciIo, SD_BAR_INDEX,SD_MMC_HC_CTRL_VER, TRUE, 2, ControllerVersion);

What is 2?
All of these '1', '2', and '4' are registers' sizes (I wouldn't write
it so many times :) ). Define will be used to improve readability.
...
...
+}



+STATIC
+VOID
+XenonSetFifo (

IN EFI_PCI_IO_PROTOCOL   *PciIo
)

+{

UINTN Data = 0x315;

#define
Ok.
...
...


SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SLOT_FIFO_CTRL, FALSE, 4, &Data);

What is 4?
...
+}



+// Auto Clock Gating
+STATIC
+VOID
+XenonSetAcg (

IN EFI_PCI_IO_PROTOCOL   *PciIo,
IN BOOLEAN Enable
)

+{

UINT32 Var;

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SYS_OP_CTRL, TRUE, 4, &Var);

What is 4?
...


if (Enable) {
Var &= ~AUTO_CLKGATE_DISABLE_MASK;
} else {
Var |= AUTO_CLKGATE_DISABLE_MASK;
}

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SYS_OP_CTRL, FALSE, 4, &Var);

What is 4?
...
+}



+STATIC
+VOID
+XenonSetSlot (

IN EFI_PCI_IO_PROTOCOL *PciIo,
IN UINT8 Slot,
IN BOOLEAN Enable
)

+{

UINT32 Var;

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SYS_OP_CTRL, TRUE, 4, &Var);

What is 4?
...

if (Enable) {
Var |= ((0x1 << Slot) << SLOT_ENABLE_SHIFT);
} else {
Var &= ~((0x1 << Slot) << SLOT_ENABLE_SHIFT);
}

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SYS_OP_CTRL, FALSE, 4, &Var);

What is 4?
...
+}



+STATIC
+VOID
+XenonSetSdio (

IN EFI_PCI_IO_PROTOCOL *PciIo,
IN UINTN Voltage
)

+{

// Currently SDIO isn't supported
return;

+}



+STATIC
+VOID
+XenonSetPower (

IN EFI_PCI_IO_PROTOCOL *PciIo,
IN UINT32 Vcc,
IN UINT32 Vccq,
IN UINT8 Mode
)

+{

UINT8 Pwr = 0;
UINT32 Ctrl = 0;

// Set VCC
switch (Vcc) {
case MMC_VDD_165_195:
Pwr = SDHCI_POWER_180;
if (Mode == XENON_MMC_MODE_SD_SDIO) {
 XenonSetSdio (PciIo, 0);



OK, so we've just determined above that SDIO isn't currently
supported. So why are we enabling/disabling it and not checking any
status?
SDIO isn't currently supported and it's stub which may be improved
(implemented correctly) in future. For now, code silently go through
this. Do you think it should be removed for now?
...
...

}
break;
case MMC_VDD_29_30:
case MMC_VDD_30_31:
Pwr = SDHCI_POWER_300;
if (Mode == XENON_MMC_MODE_SD_SDIO) {
 XenonSetSdio (PciIo, 1);


}
break;
case MMC_VDD_32_33:
case MMC_VDD_33_34:
Pwr = SDHCI_POWER_330;
if (Mode == XENON_MMC_MODE_SD_SDIO) {
 XenonSetSdio (PciIo, 1);


}
break;
default:
DEBUG((DEBUG_ERROR, "SD/MMC: Does not support power mode(0x%X)\n", Vcc));
break;
}

if (Pwr == 0) {
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_POWER_CTRL, FALSE, 1, &Pwr);

What is 1?
...

return;
}

Pwr |= SDHCI_POWER_ON;

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX,SD_MMC_HC_POWER_CTRL, FALSE, 1, &Pwr);

What is 1?
...


// Set VCCQ
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SLOT_eMMC_CTRL, TRUE, 4, &Ctrl);

What is 4?
...

Ctrl |= Vccq;
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SDHC_SLOT_eMMC_CTRL, FALSE, 4, &Ctrl);

What is 4?
...
+}



+UINTN
+XenonSetClk (

IN EFI_PCI_IO_PROTOCOL   *PciIo,
IN SD_MMC_HC_PRIVATE_DATA *Private,
IN UINT32 Clock
)

+{

UINT32 Div;
UINT32 Clk;
UINT32 Timeout;
UINT16 Value = 0;

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, FALSE, 2, &Value);

What is 2?
...


if (Clock == 0)

Always {} with if.
Ok.
...
...

return 0;

if (Private->ControllerVersion >= SDHCI_SPEC_300) {
// Version 3.00 Divisors must be a multiple of 2
if (XENON_MMC_MAX_CLK <= Clock) {
 Div = 1;


} else {
 for (Div = 2; Div < SDHCI_MAX_DIV_SPEC_300; Div += 2) {


   if ((XENON_MMC_MAX_CLK / Div) <= Clock)


     break;


 }


}
} else {
// Version 2.00 Divisors must be a power of 2
for (Div = 1; Div < SDHCI_MAX_DIV_SPEC_200; Div *= 2) {
 if ((XENON_MMC_MAX_CLK / Div) <= Clock)


   break;


}
}
Div >>= 1;

Clk = (Div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
Clk |= ((Div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN) << SDHCI_DIVIDER_HI_SHIFT;
Clk |= SDHCI_CLOCK_INT_EN;

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, FALSE, 2, &Clk);

What is 2?
...


// Wait max 20 ms

Why?
Will add more descriptive comment.
...
...

Timeout = 200;

And how does 200 delays of 1us end up being related to 20ms?
All of these mismatch between comment and delay's value are simply
that text doesn't follow with changes in code. Will review whole file
in terms of this, thanks.
...
...


do {
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, TRUE, 2, &Clk);

What is 2?
...

if (Timeout == 0) {
 DEBUG((DEBUG_ERROR, "SD/MMC: Internal Clock never stabilised\n"));


 return -1;


}

Timeout--;
gBS->Stall (1);

} while (!(Clk & SDHCI_CLOCK_INT_STABLE));

Clk |= SDHCI_CLOCK_CARD_EN;
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, FALSE, 2, &Clk);

What is 2?
...


return 0;

+}



+VOID
+XenonPhyInit (

IN EFI_PCI_IO_PROTOCOL   *PciIo
)

+{

UINT32 Var, Wait, Time;
UINT32 Clock = XENON_MMC_MAX_CLK;
UINT16 ClkCtrl;

// Need to disable the clock to set EMMC_PHY_TIMING_ADJUST register
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, TRUE, 2, &ClkCtrl);

What is 2?
...

ClkCtrl &= ~(SDHCI_CLOCK_CARD_EN | SDHCI_CLOCK_INT_EN);
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, FALSE, 2, &ClkCtrl);

What is 2?
...


// Enable QSP PHASE SELECT
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, TRUE, 4, &Var);

What is 4?
...

Var |= SAMPL_INV_QSP_PHASE_SELECT;
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, FALSE, 4, &Var);

What is 4?
...


// Enable internal clock
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, TRUE, 2, &ClkCtrl);

What is 2?
...

ClkCtrl |= SDHCI_CLOCK_INT_EN;
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, FALSE, 2, &ClkCtrl);

What is 2?
...


//
// Poll for host MMC PHY clock init to be stable

Excellent - thanks! This is exactly the kind of comment I'm looking
for in my previous comments.
...

// Wait up to 10ms
//
Time = 100;

But how does 100 x 1us end up relating to 10ms?
...

while (Time--) {
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, TRUE, 4, &Var);

What is 4?
...

if (Var & SDHCI_CLOCK_INT_STABLE) {
 break;


}

gBS->Stall (1);
}
if (Time <= 0) {
DEBUG((DEBUG_ERROR, "SD/MMC: Failed to enable MMC internal clock in Time\n"));
return;
}

// Enable bus clock
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, TRUE, 2, &ClkCtrl);

What is 2?
...

ClkCtrl |= SDHCI_CLOCK_CARD_EN;
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_CLOCK_CTRL, FALSE, 2, &ClkCtrl);

What is 2?
...


// Delay 200us to Wait for the completion of bus clock
gBS->Stall (200);

// Init PHY
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, TRUE, 4, &Var);

What is 4?
...

Var |= PHY_INITIALIZAION;
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, FALSE, 4, &Var);

What is 4?
(OK, I'll stop now, but please provide some #define conveying what the
actual significance it.)
...


// Add duration of FC_SYNC_RST
Wait = ((Var >> FC_SYNC_RST_DURATION_SHIFT) & FC_SYNC_RST_DURATION_MASK);

// Add interval between FC_SYNC_EN and FC_SYNC_RST
Wait += ((Var >> FC_SYNC_RST_EN_DURATION_SHIFT) & FC_SYNC_RST_EN_DURATION_MASK);

// Add duration of asserting FC_SYNC_EN
Wait += ((Var >> FC_SYNC_EN_DURATION_SHIFT) & FC_SYNC_EN_DURATION_MASK);

// Add duration of Waiting for PHY
Wait += ((Var >> WAIT_CYCLE_BEFORE_USING_SHIFT) & WAIT_CYCLE_BEFORE_USING_MASK);

// 4 addtional bus clock and 4 AXI bus clock are required left shift 20 bits
Wait += 8;
Wait <<= 20;

// Use the possibly slowest bus frequency value
if (Clock == 0) {
Clock = 100000;

#define?
Ok.
...
...

}

// Get the Wait Time in unit of ms
Wait = Wait / Clock;
Wait++;

// Poll for host eMMC PHY init to complete, wait up to 10ms
Time = 100;

Still not getting the 1 x 100 == 10000.
As above.
...
...

while (Time--) {
Var = SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, TRUE, 4, &Var);
Var &= PHY_INITIALIZAION;
if (!Var) {
 break;


}

// Wait for host eMMC PHY init to complete
gBS->Stall (1);
}

if (Time <= 0) {
DEBUG((DEBUG_ERROR, "SD/MMC: Failed to init MMC PHY in Time\n"));
return;
}

return;

+}



+STATIC
+VOID
+XenonSetPhy (

IN EFI_PCI_IO_PROTOCOL   *PciIo,
UINT8 Timing
)

+{

UINT32 Var = 0;

// Setup pad, set bit[30], bit[28] and bits[26:24]
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_PAD_CONTROL, TRUE, 4, &Var);
Var |= (AUTO_RECEN_CTRL | OEN_QSN | FC_QSP_RECEN | FC_CMD_RECEN | FC_DQ_RECEN);
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, EMMC_PHY_PAD_CONTROL, FALSE, 4, &Var);

//
// If Timing belongs to high speed, set bit[17] of
// EMMC_PHY_TIMING_ADJUST register
//
if ((Timing == MMC_TIMING_MMC_HS400) ||
 (Timing == MMC_TIMING_MMC_HS200) ||


 (Timing == MMC_TIMING_UHS_SDR50) ||


 (Timing == MMC_TIMING_UHS_SDR104) ||


 (Timing == MMC_TIMING_UHS_DDR50) ||


 (Timing == MMC_TIMING_UHS_SDR25)) {



SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, TRUE, 4, &Var);

// Set SLOW_MODE for PHY
Var |= OUTPUT_QSN_PHASE_SELECT | (1 << 29);

#define?
Ok, as I wrote above, we will replace comments with #defines.
...
...

SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_PHY_TIMING_ADJUST, FALSE, 4, &Var);
}

SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_PHY_FUNC_CONTROL, TRUE, 4, &Var);
Var |= (DQ_DDR_MODE_MASK << DQ_DDR_MODE_SHIFT) | CMD_DDR_MODE;
SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_PHY_FUNC_CONTROL, FALSE, 4, &Var);

if (Timing == MMC_TIMING_MMC_HS400) {
SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_PHY_FUNC_CONTROL, TRUE, 4, &Var);
Var &= ~DQ_ASYNC_MODE;
SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_PHY_FUNC_CONTROL, FALSE, 4, &Var);

Var = LOGIC_TIMING_VALUE;
SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, EMMC_LOGIC_TIMING_ADJUST, FALSE, 4, &Var);
}

XenonPhyInit (PciIo);

+}



+STATIC
+VOID
+XenonConfigureInterrupts (

IN EFI_PCI_IO_PROTOCOL *PciIo
)

+{

UINT32 Var;

// Clear interrupt status
Var = 0xFFFFFFFF;

UINT_MAX? ~0?
Ok
...
...

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_NOR_INT_STS, FALSE, 4, &Var);
SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_NOR_INT_STS, FALSE, 4, &Var);

// Enable only interrupts served by the SD controller
Var = 0xFFFFFEBF;

Some #define and a ~?
Ok
...
...

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_NOR_INT_STS_EN, FALSE, 4, &Var);

// Mask all sdhci interrupt sources
Var = 0xFFFFFEFF;

Some #define and a ~?
Ok
...
...

SdMmcHcRwMmio (PciIo, SD_BAR_INDEX, SD_MMC_HC_NOR_INT_SIG_EN, FALSE, 4, &Var);

+}



+// Enable Parallel Transfer Mode
+STATIC
+VOID
+XenonSetParallelTransfer (

IN EFI_PCI_IO_PROTOCOL *PciIo,
IN UINT8 Slot,
IN BOOLEAN Enable
)

+{

UINT32 Var;

SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, SDHC_SYS_EXT_OP_CTRL, TRUE, 4, &Var);

if (Enable) {
Var |= (0x1 << Slot);
} else {
Var &= ~(0x1 << Slot);
}

SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, SDHC_SYS_EXT_OP_CTRL, FALSE, 4, &Var);

+}



+STATIC
+VOID
+XenonSetTuning (

IN EFI_PCI_IO_PROTOCOL   *PciIo,
IN UINT8 Slot,
IN BOOLEAN Enable
)

+{

UINT32 Var;

// Set the Re-Tuning Request functionality
SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, SDHC_SLOT_RETUNING_REQ_CTRL, TRUE, 4, &Var);

if (Enable) {
Var |= RETUNING_COMPATIBLE;
} else {
Var &= ~RETUNING_COMPATIBLE;
}

SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, SDHC_SLOT_RETUNING_REQ_CTRL, FALSE, 4, &Var);

// Set the Re-tuning Event Signal Enable
SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, SDHCI_SIGNAL_ENABLE, TRUE, 4, &Var);

if (Enable) {
Var |= SDHCI_RETUNE_EVT_INTSIG;
} else {
Var &= ~SDHCI_RETUNE_EVT_INTSIG;
}

SdMmcHcRwMmio(PciIo, SD_BAR_INDEX, SDHCI_SIGNAL_ENABLE, FALSE, 4, &Var);

+}



+VOID
+XenonReset (

IN SD_MMC_HC_PRIVATE_DATA *Private,
IN UINT8 Slot,
IN UINT8 Mask
)

+{

UINT32 Timeout = 1000;

Retries?
Rigth, it's better.
...
...

UINT8 SwReset;

SwReset = Mask;

SdMmcHcRwMmio (
     Private->PciIo,


     Slot,


     SD_MMC_HC_SW_RST,


     FALSE,


     sizeof (SwReset),


     &SwReset


   );



SdMmcHcRwMmio (
     Private->PciIo,


     Slot,


     SD_MMC_HC_SW_RST,


     TRUE,


     sizeof (SwReset),


     &SwReset


   );



while (SwReset & Mask) {
if (Timeout == 0) {
 DEBUG((DEBUG_ERROR, "SD/MMC: Reset never completed\n"));


 return;


}

Timeout--;
gBS-> Stall (100);
SdMmcHcRwMmio (
       Private->PciIo,


       Slot,


       SD_MMC_HC_SW_RST,


       TRUE,


       sizeof (SwReset),


       &SwReset


     );


}

+}



+STATIC
+VOID
+XenonTransferPio (

IN SD_MMC_HC_PRIVATE_DATA *Private,
IN UINT8 Slot,
IN OUT VOID *Buffer,
IN UINT16 BlockSize,
IN BOOLEAN Read
)

+{

UINTN I;
UINT8 *Offs;

//
// SD stack's intrinsic functions cannot perform properly reading/writing from
// buffer register, that is why MmioRead/MmioWrite are used. It is temporary
// solution.
//
for (I = 0; I < BlockSize; I += 4) {
Offs = Buffer + I;
if (Read) {
 *(UINT32 *)Offs = MmioRead32 (0xf06e0020); // SDHCI_BUFFER



#define.
Ok.
...
...

} else {
 MmioWrite32 (0xf06e0020, *(UINT32 *)Offs); // SDHCI_BUFFER



#define.
Also - this driver is still doing PCI semantics, but now it's doing
direct access to a hard-coded memory address. That's not very pretty.
Can we either obtain this information in a more dynamic fasion, or cut
away every last mention of PCI?
Comment above for loop specify why we use such approach. Will revisit
and try to debug one more time, but if it doesn't help we will use
consistent approach.
...
...

}
}

+}



+EFI_STATUS
+XenonTransferData (

IN SD_MMC_HC_PRIVATE_DATA *Private,
IN UINT8 Slot,
IN OUT VOID *Buffer,
IN UINT32 DataLen,
IN UINT16 BlockSize,
IN UINT16 Blocks,
IN BOOLEAN Read
)

+{

UINT32 IntStatus, PresentState, Rdy, Mask, Timeout, Block = 0;

if (Buffer == NULL) {
return EFI_DEVICE_ERROR;
}

Timeout = 100000; // 100000 * 10 us = 1 second

This line inserted to clarify below comment does not apply to above.
Agree.
...
...

Rdy = 0x10 | 0x20; // SDHCI_INT_SPACE_AVAIL | SDHCI_INT_DATA_AVAIL
Mask = 0x800 | 0x400; // SDHCI_DATA_AVAILABLE | SDHCI_SPACE_AVAILABLE

#defines instead of comments?
Ok.
...
...


do {
SdMmcHcRwMmio (
       Private->PciIo,


       Slot,


       SD_MMC_HC_NOR_INT_STS,


       TRUE,


       sizeof (IntStatus),


       &IntStatus


     );



if (IntStatus & BIT15) { // SDHCI_INT_ERROR

#define instead of comment?
...

 DEBUG((DEBUG_INFO, "SD/MMC: Error detected in status %0x\n", IntStatus));


 return EFI_DEVICE_ERROR;


}

if (IntStatus & Rdy) {
 SdMmcHcRwMmio (


         Private->PciIo,


         Slot,


         SD_MMC_HC_PRESENT_STATE,


         TRUE,


         sizeof (PresentState),


         &PresentState


       );



 if (!(PresentState & Mask)) {


   continue;


 }



 SdMmcHcRwMmio (


         Private->PciIo,


         Slot,


         SD_MMC_HC_NOR_INT_STS,


         FALSE,


         sizeof (Rdy),


         &Rdy


       );



 XenonTransferPio (Private, Slot, Buffer, BlockSize, Read);



 Buffer += BlockSize;


 if (++Block >= Blocks) {


   break;


 }


}

if (Timeout-- > 0) {
 gBS->Stall (10);


} else {
 DEBUG((DEBUG_INFO, "SD/MMC: Transfer data timeout\n"));


 return EFI_TIMEOUT;


}
} while (!(IntStatus & BIT1)); // SDHCI_INT_DATA_END

#define instead of comment?
(Oh, and please add blank line before return.
Ok.
...
...

return EFI_SUCCESS;

+}



+EFI_STATUS
+XenonInit (

IN SD_MMC_HC_PRIVATE_DATA *Private
)

+{

EFI_PCI_IO_PROTOCOL *PciIo = Private->PciIo;

XenonSetMpp ();

XenonReadVersion (PciIo, &Private->ControllerVersion);

XenonSetFifo (PciIo);

XenonSetAcg (PciIo, FALSE);

The blank lines between every function call actually makes this
function a bit difficult to read. Can calls be grouped as logically
connected? Or more comments added?
I think adding more comments will make it looks better.
...
...


// Hyperion has only one port
XenonSetSlot (PciIo, XENON_MMC_SLOT_ID_HYPERION, TRUE);

XenonSetPower (PciIo, MMC_VDD_165_195, eMMC_VCCQ_1_8V, XENON_MMC_MODE_SD_SDIO);

// Set MAX_CLOCK for configuring PHY
XenonSetClk (PciIo, Private, XENON_MMC_MAX_CLK);

XenonSetPhy (PciIo, MMC_TIMING_UHS_SDR50);

XenonConfigureInterrupts (PciIo);

// Enable parallel transfer
XenonSetParallelTransfer (PciIo, XENON_MMC_SLOT_ID_HYPERION, TRUE);

XenonSetTuning (PciIo, XENON_MMC_SLOT_ID_HYPERION, FALSE);

XenonSetAcg (PciIo, TRUE);

XenonSetClk (PciIo, Private, XENON_MMC_BASE_CLK);

XenonPhyInit (PciIo);

return EFI_SUCCESS;

+}
diff --git a/Drivers/SdMmc/XenonSdhci.h b/Drivers/SdMmc/XenonSdhci.h
new file mode 100644
index 0000000..a7b30c3
--- /dev/null
+++ b/Drivers/SdMmc/XenonSdhci.h
@@ -0,0 +1,278 @@
+/*******************************************************************************
+Copyright (C) 2016 Marvell International Ltd.



+Marvell BSD License Option



+If you received this File from Marvell, you may opt to use, redistribute and/or
+modify this File under the following licensing terms.
+Redistribution and use in source and binary forms, with or without modification,
+are permitted provided that the following conditions are met:



+* Redistributions of source code must retain the above copyright notice,

this list of conditions and the following disclaimer.


+* Redistributions in binary form must reproduce the above copyright

notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.


+* Neither the name of Marvell nor the names of its contributors may be

used to endorse or promote products derived from this software without
specific prior written permission.


+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.



+*******************************************************************************/



+#include "../../../MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h"
Use local copy.
Ok.
...
...



+#include <Library/IoLib.h>



+#define SD_BAR_INDEX 0



+/* Register Offset of SD Host Controller SOCP self-defined register */



+#define SDHC_IPID                     0x0100
+#define SDHC_SYS_CFG_INFO             0x0104
+#define SLOT_TYPE_SDIO_SHIFT          24
+#define SLOT_TYPE_EMMC_MASK           0xff
+#define SLOT_TYPE_EMMC_SHIFT          16
+#define SLOT_TYPE_SD_SDIO_MMC_MASK    0xff
+#define SLOT_TYPE_SD_SDIO_MMC_SHIFT   8



+#define SDHC_SYS_OP_CTRL              0x0108
+#define AUTO_CLKGATE_DISABLE_MASK     (0x1<<20)
+#define SDCLK_IDLEOFF_ENABLE_SHIFT    8
+#define SLOT_ENABLE_SHIFT             0



+#define SDHC_SYS_EXT_OP_CTRL          0x010c
+#define SDHC_TEST_OUT                 0x0110
+#define SDHC_TESTOUT_MUXSEL           0x0114



+#define SDHC_SLOT_EXT_INT_STATUS      0x0120
+#define SDHC_SLOT_EXT_ERR_STATUS      0x0122
+#define SDHC_SLOT_EXT_INT_STATUS_EN   0x0124
+#define SDHC_SLOT_EXT_ERR_STATUS_EN   0x0126



+#define SDHC_SLOT_OP_STATUS_CTRL      0x0128
+#define TUNING_PROG_FIXED_DELAY_MASK  0x7ff
+#define FORCE_SEL_INVERSE_CLK_SHIFT   11



+#define SDHC_SLOT_FIFO_CTRL           0x012c



+#define SDHC_SLOT_eMMC_CTRL           0x0130
+#define ENABLE_DATA_STROBE_SHIFT      24
+#define SET_EMMC_RSTN_SHIFT           16
+#define eMMC_VCCQ_MASK                0x3
+#define eMMC_VCCQ_1_8V                0x1
+#define eMMC_VCCQ_1_2V                0x2
+#define eMMC_VCCQ_3_3V                0x3



+#define SDHC_SLOT_OUTPUT_DLY_CTRL     0x0134
+#define SDHC_SLOT_DCM_CTRL            0x0137



+#define SDHC_SLOT_DLL_CTRL            0x0138
+#define SELECT_DEF_DLL                0x1



+#define SDHC_SLOT_DLL_PHASE_SEL       0x013c
+#define DLL_UPDATE_STROBE             7



+#define SDHC_SLOT_STROBE_DLY_CTRL     0x0140
+#define STROBE_DELAY_FIXED_MASK       0xffff



+#define SDHC_SLOT_RETUNING_REQ_CTRL   0x0144
+#define RETUNING_COMPATIBLE           0x1



+#define SDHC_SLOT_AUTO_RETUNING_CTRL  0x0148
+#define ENABLE_AUTO_RETUNING          0x1



+#define SDHC_SLOT_EXT_PRESENT_STATE   0x014c
+#define SDHC_SLOT_DLL_CUR_DLY_VAL     0x0150
+#define SDHC_SLOT_TUNING_CUR_DLY_VAL  0x0154
+#define SDHC_SLOT_STROBE_CUR_DLY_VAL  0x0158
+#define SDHC_SLOT_SUB_CMD_STRL        0x015c



+#define SDHC_SLOT_CQ_TASK_INFO        0x0160



+#define SDHC_SLOT_TUNING_DEBUG_INFO   0x01f0
+#define SDHC_SLOT_DATAIN_DEBUG_INFO   0x01f4



+#define XENON_MMC_MAX_CLK             400000000
HZ? (A comment would suffice.)
Ok.
...
...
+#define XENON_MMC_CLK_RATIO           2046
+#define XENON_MMC_BASE_CLK            XENON_MMC_MAX_CLK / XENON_MMC_CLK_RATIO



+#define XENON_MMC_CMD_MAX_TIMEOUT     3200
+#define XENON_MMC_CMD_DEFAULT_TIMEOUT 100



+/* Tuning Parameter */
+#define TMR_RETUN_NO_PRESENT          0xf
+#define XENON_MAX_TUN_COUNT           0xb



+#define EMMC_PHY_REG_BASE                 0x170
+#define EMMC_PHY_TIMING_ADJUST            EMMC_PHY_REG_BASE
+#define OUTPUT_QSN_PHASE_SELECT           (1 << 17)
+#define SAMPL_INV_QSP_PHASE_SELECT        (1 << 18)
+#define SAMPL_INV_QSP_PHASE_SELECT_SHIFT  18
+#define PHY_INITIALIZAION                 (1 << 31)
+#define WAIT_CYCLE_BEFORE_USING_MASK      0xf
+#define WAIT_CYCLE_BEFORE_USING_SHIFT     12
+#define FC_SYNC_EN_DURATION_MASK          0xf
+#define FC_SYNC_EN_DURATION_SHIFT         8
+#define FC_SYNC_RST_EN_DURATION_MASK      0xf
+#define FC_SYNC_RST_EN_DURATION_SHIFT     4
+#define FC_SYNC_RST_DURATION_MASK         0xf
+#define FC_SYNC_RST_DURATION_SHIFT        0



+#define EMMC_PHY_FUNC_CONTROL     (EMMC_PHY_REG_BASE + 0x4)
+#define DQ_ASYNC_MODE             (1 << 4)
+#define DQ_DDR_MODE_SHIFT         8
+#define DQ_DDR_MODE_MASK          0xff
+#define CMD_DDR_MODE              (1 << 16)



+#define EMMC_PHY_PAD_CONTROL      (EMMC_PHY_REG_BASE + 0x8)
+#define REC_EN_SHIFT              24
+#define REC_EN_MASK               0xf
+#define FC_DQ_RECEN               (1 << 24)
+#define FC_CMD_RECEN              (1 << 25)
+#define FC_QSP_RECEN              (1 << 26)
+#define FC_QSN_RECEN              (1 << 27)
+#define OEN_QSN                   (1 << 28)
+#define AUTO_RECEN_CTRL           (1 << 30)



+#define EMMC_PHY_PAD_CONTROL1        (EMMC_PHY_REG_BASE + 0xc)
+#define EMMC_PHY_PAD_CONTROL2        (EMMC_PHY_REG_BASE + 0x10)
+#define EMMC_PHY_DLL_CONTROL         (EMMC_PHY_REG_BASE + 0x14)
+#define DLL_DELAY_TEST_LOWER_SHIFT   8
+#define DLL_DELAY_TEST_LOWER_MASK    0xff
+#define DLL_BYPASS_EN                0x1



+#define EMMC_LOGIC_TIMING_ADJUST       (EMMC_PHY_REG_BASE + 0x18)
+#define EMMC_LOGIC_TIMING_ADJUST_LOW   (EMMC_PHY_REG_BASE + 0x1c)



+#define MVEBU_IP_CONFIG_REG            (0xf0000000 + 0x6F4100)
Again, slightly uncomfortable with this hard-coded address. (But
depending on what you do about my comment on where this is actually
used, could possibly be dropped.)
Yes, It will be removed from there.
...
...



+#define LOGIC_TIMING_VALUE             0x5a54 /* Recommend by HW team */



+/* Hyperion only have one slot 0 */
What is Hyperion? Internal name of a specific devboard?
It's old name and should be removed

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

Re: [Linaro-uefi] [PATCH 2/4] Drivers/SdMmc: Adjust SdMmcPciHcDxe to fulfill Xenon controller demands