2017-07-06 23:22 GMT+08:00 Leif Lindholm <leif.lindh...@linaro.org>:
> On Wed, Jul 05, 2017 at 04:27:08PM +0800, Jun Nie wrote:
>> Adjust FIFO threshold according to FIFO depth. Skip
>> the adjustment if we do not have FIFO depth info.
>>
>
> So, this is a big improvement in readability - but some of my generic
> style comments do not appear to have been addressed.
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jun Nie <jun....@linaro.org>
>> ---
>> EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h | 6 ++++
>> EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 50
>> +++++++++++++++++++++++++++++
>> EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf | 1 +
>> EmbeddedPkg/EmbeddedPkg.dec | 1 +
>> 4 files changed, 58 insertions(+)
>>
>> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
>> b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
>> index 055f1e0..90c7676 100644
>> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
>> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmc.h
>> @@ -38,7 +38,10 @@
>> #define DWEMMC_RINTSTS ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x044)
>> #define DWEMMC_STATUS ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x048)
>> #define DWEMMC_FIFOTH ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x04c)
>> +#define DWEMMC_TCBCNT ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x05c)
>> +#define DWEMMC_TBBCNT ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x060)
>> #define DWEMMC_DEBNCE ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x064)
>> +#define DWEMMC_HCON ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x070)
>> #define DWEMMC_UHSREG ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x074)
>> #define DWEMMC_BMOD ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x080)
>> #define DWEMMC_DBADDR ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x088)
>> @@ -47,6 +50,7 @@
>> #define DWEMMC_DSCADDR ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x094)
>> #define DWEMMC_BUFADDR ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0x098)
>> #define DWEMMC_CARDTHRCTL ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0X100)
>> +#define DWEMMC_DATA ((UINT32)PcdGet32 (PcdDwEmmcDxeBaseAddress)
>> + 0X200)
>>
>> #define CMD_UPDATE_CLK 0x80202000
>> #define CMD_START_BIT (1 << 31)
>> @@ -124,4 +128,6 @@
>> #define DWEMMC_CARD_RD_THR(x) ((x & 0xfff) << 16)
>> #define DWEMMC_CARD_RD_THR_EN (1 << 0)
>>
>> +#define DWEMMC_GET_HDATA_WIDTH(x) (((x) >> 7) & 0x7)
>> +
>> #endif // __DWEMMC_H__
>> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
>> b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
>> index bb26b69..70e064d 100644
>> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
>> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
>> @@ -415,6 +415,55 @@ DwEmmcReceiveResponse (
>> return EFI_SUCCESS;
>> }
>>
>> +VOID DwEmmcAdjustFifothreshold (
>
> This still needs to be:
> VOID
> DwEmmcAdjustFifoThreshold (
>
>> + VOID
>> + )
>> +{
>> + /* DMA multiple transaction size map to reg value as array index */
>> + CONST UINT32 BurstSize[] = {1, 4, 8, 16, 32, 64, 128, 256};
>> + UINT32 BlkDepthInFifo, Fifoth, FifoWidth, FifoDepth;
>
> Fifoth -> FifoThreshold.
>
>> + UINT32 BlkSize = DWEMMC_BLOCK_SIZE, Idx = 0, RxWmark = 1, TxWmark,
>> TxWmarkInvers;
>
> RxWmark -> RxWaterMark.
> TxWmark -> TxWaterMark.
> TxWmarkInvers -> TxWaterMarkInverse.
My fault. The style is totally different with kernel and I am not
fully customized yet.
>
>> +
>> + /* Skip FIFO adjustment if we do not have platform FIFO depth info */
>> + FifoDepth = PcdGet32 (PcdDwEmmcDxeFifoDepth);
>> + if (!FifoDepth) {
>> + return;
>> + }
>> +
>> + TxWmark = FifoDepth / 2;
>> + TxWmarkInvers = FifoDepth - TxWmark;
>> +
>> + FifoWidth = DWEMMC_GET_HDATA_WIDTH (MmioRead32 (DWEMMC_HCON));
>> + if (!FifoWidth) {
>> + FifoWidth = 2;
>> + } else if (FifoWidth == 2) {
>> + FifoWidth = 8;
>> + } else {
>> + FifoWidth = 4;
>> + }
>> +
>> + BlkDepthInFifo = BlkSize / FifoWidth;
>> +
>> + /* if BlkSize is not a multiple of the FIFO width */
>
> This comment still only says exactly what the code says.
> Why are we done if BlkSize is not a multiple of FifoWidth?
> Is this situation possible? What does it indicate?
You find the key point. As this driver is dedicated for EMMC, not
shared with MMC, this check shall never be true. We can ignore it.
Jun
>
> /
> Leif
>
>> + if (BlkSize % FifoWidth) {
>> + goto done;
>> + }
>> +
>> + Idx = ARRAY_SIZE (BurstSize) - 1;
>> + while (Idx && ((BlkDepthInFifo % BurstSize[Idx]) || (TxWmarkInvers %
>> BurstSize[Idx]))) {
>> + Idx--;
>> + }
>> + RxWmark = BurstSize[Idx] - 1;
>> + /*
>> + * If Idx is '0', it won't be tried
>> + * Thus, initial values are used
>> + */
>> +done:
>> + Fifoth = DWEMMC_DMA_BURST_SIZE (Idx) | DWEMMC_FIFO_TWMARK (TxWmark)
>> + | DWEMMC_FIFO_RWMARK (RxWmark);
>> + MmioWrite32 (DWEMMC_FIFOTH, Fifoth);
>> +}
>> +
>> EFI_STATUS
>> PrepareDmaData (
>> IN DWEMMC_IDMAC_DESCRIPTOR* IdmacDesc,
>> @@ -633,6 +682,7 @@ DwEmmcDxeInitialize (
>>
>> Handle = NULL;
>>
>> + DwEmmcAdjustFifothreshold ();
>> gpIdmacDesc = (DWEMMC_IDMAC_DESCRIPTOR *)AllocatePages
>> (DWEMMC_MAX_DESC_PAGES);
>> if (gpIdmacDesc == NULL) {
>> return EFI_BUFFER_TOO_SMALL;
>> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
>> b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
>> index 99b4f99..bc4413e 100644
>> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
>> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
>> @@ -49,6 +49,7 @@
>> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress
>> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz
>> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz
>> + gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeFifoDepth
>>
>> [Depex]
>> TRUE
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>> index 2da9b2f..5f39d9d 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>> @@ -170,6 +170,7 @@
>> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0x0|UINT32|0x00000035
>>
>> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|0x0|UINT32|0x00000036
>> gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeMaxClockFreqInHz|0x0|UINT32|0x00000037
>> + gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeFifoDepth|0x0|UINT32|0x00000038
>>
>> #
>> # Android FastBoot
>> --
>> 1.9.1
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
