public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v4 00/11] enhance MMC
@ 2016-11-08 15:21 Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 01/11] MmcDxe: wait OCR busy bit free Haojian Zhuang
                   ` (11 more replies)
  0 siblings, 12 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

v4:
  * Fix PL180 hang in some cases. Since the proper variable length
    isn't set for CMD6 & ACMD51.

v3:
  * Fix PL180 hang because of CMD6 & ACMD51 not supported.

v2:
  * Fix print error with missing parameter.
  * Change CMD51 to ACMD51.
  * Add the protection after CMD55 for SD. If there's no
    response of CMD55, skip to send ACMD51.

v1:
  * Wait OCR busy bit free according to eMMC spec.
  * Define ECSD structure.
  * Add interface to set IO bus width and speed.
  * Support to access multiple blocks.

Haojian Zhuang (11):
  MmcDxe: wait OCR busy bit free
  MmcDxe: move ECSD into CardInfo structure
  MmcDxe: add SPEC_VERS field in CSD structure
  MmcDxe: add interface to change io width and speed
  MmcDxe: declare ECSD structure
  MmcDxe: set io bus width before reading EXTCSD
  MmcDxe: Fix uninitialized mediaid for SD
  MmcDxe: set iospeed and bus width in SD stack
  MmcDxe: expand to support multiple blocks
  PL180: update for indentifying SD
  PL180: set variable length for CMD6 and ACMD51

 ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c    |  29 +-
 EmbeddedPkg/Include/Protocol/MmcHost.h           |  29 ++
 EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 176 ++++++++++-
 EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c        | 175 +++++++----
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 354 +++++++++++++++++++++--
 5 files changed, 669 insertions(+), 94 deletions(-)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v4 01/11] MmcDxe: wait OCR busy bit free
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 19:27   ` Leif Lindholm
  2016-11-08 15:21 ` [PATCH v4 02/11] MmcDxe: move ECSD into CardInfo structure Haojian Zhuang
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

According to eMMC spec, OCR.PowerUp bit is also busy bit. If the busy
bit is '0', CMD1 should be sent and OCR should be fetched again. And add
a timeout counter on the repeated steps.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 2d8038f..3f72b7f 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -222,14 +222,19 @@ MmcIdentificationMode (
 
   // Send CMD1 to get OCR (MMC)
   // This command only valid for MMC and eMMC
-  Status = MmcHost->SendCommand (MmcHost, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
-  if (Status == EFI_SUCCESS) {
+  Timeout = MAX_RETRY_COUNT;
+  do {
+    Status = MmcHost->SendCommand (MmcHost, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
+    if (EFI_ERROR (Status))
+      break;
     Status = MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_OCR, (UINT32 *)&OcrResponse);
     if (EFI_ERROR (Status)) {
       DEBUG ((EFI_D_ERROR, "MmcIdentificationMode() : Failed to receive OCR, Status=%r.\n", Status));
       return Status;
     }
-
+    Timeout--;
+  } while (!OcrResponse.Ocr.PowerUp && (Timeout > 0));
+  if (Status == EFI_SUCCESS) {
     if (!OcrResponse.Ocr.PowerUp) {
       DEBUG ((EFI_D_ERROR, "MmcIdentificationMode(MMC_CMD1): Card initialisation failure, Status=%r.\n", Status));
       return EFI_DEVICE_ERROR;
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 02/11] MmcDxe: move ECSD into CardInfo structure
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 01/11] MmcDxe: wait OCR busy bit free Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 20:27   ` Leif Lindholm
  2016-11-08 15:21 ` [PATCH v4 03/11] MmcDxe: add SPEC_VERS field in CSD structure Haojian Zhuang
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Since ECSD also describes the information of card, move it into
structure CardInfo.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 1 +
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
index 46a156c..4f132c6 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
@@ -138,6 +138,7 @@ typedef struct  {
   OCR       OCRData;
   CID       CIDData;
   CSD       CSDData;
+  UINT32    ECSD[128];      // MMC V4 extended card specific
 } CARD_INFO;
 
 typedef struct _MMC_HOST_INSTANCE {
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 3f72b7f..578fcb6 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -39,7 +39,6 @@ EmmcIdentificationMode (
   EFI_BLOCK_IO_MEDIA    *Media;
   EFI_STATUS Status;
   UINT32     RCA;
-  UINT32     ECSD[128];
 
   Host  = MmcHostInstance->MmcHost;
   Media = MmcHostInstance->BlockIo.Media;
@@ -91,7 +90,7 @@ EmmcIdentificationMode (
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
   }
 
-  Status = Host->ReadBlockData (Host, 0, 512, ECSD);
+  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSD));
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
     return Status;
@@ -104,7 +103,7 @@ EmmcIdentificationMode (
   Media->LogicalBlocksPerPhysicalBlock = 1;
   Media->IoAlign = 4;
   // Compute last block using bits [215:212] of the ECSD
-  Media->LastBlock = ECSD[EMMC_ECSD_SIZE_OFFSET] - 1; // eMMC isn't supposed to report this for
+  Media->LastBlock = MmcHostInstance->CardInfo.ECSD[EMMC_ECSD_SIZE_OFFSET] - 1; // eMMC isn't supposed to report this for
   // Cards <2GB in size, but the model does.
 
   // Setup card type
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 03/11] MmcDxe: add SPEC_VERS field in CSD structure
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 01/11] MmcDxe: wait OCR busy bit free Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 02/11] MmcDxe: move ECSD into CardInfo structure Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 20:27   ` Leif Lindholm
  2016-11-08 15:21 ` [PATCH v4 04/11] MmcDxe: add interface to change io width and speed Haojian Zhuang
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Universal/MmcDxe/Mmc.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
index 4f132c6..8f30a9a 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
@@ -128,7 +128,8 @@ typedef struct {
   UINT8   NSAC                ;  // Data read access-time 2 in CLK cycles (NSAC*100) [111:104]
   UINT8   TAAC                ;  // Data read access-time 1 [119:112]
 
-  UINT8   RESERVED_5:         6; // Reserved [125:120]
+  UINT8   RESERVED_5:         2; // Reserved [121:120]
+  UINT8   SPEC_VERS:          4; // System specification version [125:122]
   UINT8   CSD_STRUCTURE:      2; // CSD structure [127:126]
 } CSD;
 
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 04/11] MmcDxe: add interface to change io width and speed
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (2 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 03/11] MmcDxe: add SPEC_VERS field in CSD structure Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 05/11] MmcDxe: declare ECSD structure Haojian Zhuang
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

By default, MMC is initialized with 1-bit mode and less than 400KHz bus
clock. It causes MMC working inefficiently.

Add the interface to change the bus width and speed.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Include/Protocol/MmcHost.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
index 89f2e80..a242291 100644
--- a/EmbeddedPkg/Include/Protocol/MmcHost.h
+++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
@@ -131,6 +131,12 @@ typedef EFI_STATUS (EFIAPI *MMC_WRITEBLOCKDATA) (
   IN  UINT32                    *Buffer
   );
 
+typedef EFI_STATUS (EFIAPI *MMC_SETIOS) (
+  IN  EFI_MMC_HOST_PROTOCOL     *This,
+  IN  UINT32                    BusClockRate,
+  IN  UINT32                    BusWidth
+  );
+
 
 struct _EFI_MMC_HOST_PROTOCOL {
 
@@ -147,6 +153,8 @@ struct _EFI_MMC_HOST_PROTOCOL {
   MMC_READBLOCKDATA       ReadBlockData;
   MMC_WRITEBLOCKDATA      WriteBlockData;
 
+  MMC_SETIOS              SetIos;
+
 };
 
 #define MMC_HOST_PROTOCOL_REVISION    0x00010001    // 1.1
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 05/11] MmcDxe: declare ECSD structure
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (3 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 04/11] MmcDxe: add interface to change io width and speed Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 20:43   ` Leif Lindholm
  2016-11-08 15:21 ` [PATCH v4 06/11] MmcDxe: set io bus width before reading EXTCSD Haojian Zhuang
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Declare fields in ECSD structure. And drop the original 128 words
arrary.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 157 ++++++++++++++++++++++-
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c |   4 +-
 2 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
index 8f30a9a..d3e9b71 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
@@ -133,13 +133,168 @@ typedef struct {
   UINT8   CSD_STRUCTURE:      2; // CSD structure [127:126]
 } CSD;
 
+typedef struct {
+  UINT8   RESERVED_1[16];                     // Reserved [15:0]
+  UINT8   SECURE_REMOVAL_TYPE;                // Secure Removal Type [16:16]
+  UINT8   PRODUCT_STATE_AWARENESS_ENABLEMENT; // Product state awareness enablement [17:17]
+  UINT8   MAX_PRE_LOADING_DATA_SIZE[4];       // MAX pre loading data size [21:18]
+  UINT8   PRE_LOADING_DATA_SIZE[4];           // Pre loading data size [25:22]
+  UINT8   FFU_STATUS;                         // FFU Status [26:26]
+  UINT8   RESERVED_2[2];                      // Reserved [28:27]
+  UINT8   MODE_OPERATION_CODES;               // Mode operation codes [29:29]
+  UINT8   MODE_CONFIG;                        // Mode config [30:30]
+  UINT8   RESERVED_3;                         // Reserved [31:31]
+  UINT8   FLUSH_CACHE;                        // Flushing of the cache [32:32]
+  UINT8   CACHE_CTRL;                         // Control to turn the cache ON/OFF [33:33]
+  UINT8   POWER_OFF_NOTIFICATION;             // Power Off Notification [34:34]
+  UINT8   PACKED_FAILURE_INDEX;               // Packed command failure index [35:35]
+  UINT8   PACKED_COMMAND_STATUS;              // Packed command status [36:36]
+  UINT8   CONTEXT_CONF[15];                   // Context configuration [51:37]
+  UINT8   EXT_PARTITIONS_ATTRIBUTE[2];        // Extended partitions attribute [53:52]
+  UINT8   EXCEPTION_EVENTS_STATUS[2];         // Exception events status [55:54]
+  UINT8   EXCEPTION_EVENTS_CTRL[2];           // Exception events control [57:56]
+  UINT8   DYNCAP_NEEDED;                      // Number of addressed group to be released [58:58]
+  UINT8   CLASS_6_CTRL;                       // Class 6 commands control [59:59]
+  UINT8   INI_TIMEOUT_EMU;                    // 1st initialization after disabling sector size emulation [60:60]
+  UINT8   DATA_SECTOR_SIZE;                   // Sector size [61:61]
+  UINT8   USE_NATIVE_SECTOR;                  // Sector size emulation [62:62]
+  UINT8   NATIVE_SECTOR_SIZE;                 // Native sector size [63:63]
+  UINT8   VENDOR_SPECIFIC_FIELD[64];          // Vendor specific fields [127:64]
+  UINT8   RESERVED_4[2];                      // Reserved [129:128]
+  UINT8   PROGRAM_CID_CSD_DDR_SUPPORT;        // Program CID/CSD in DDR mode support [130:130]
+  UINT8   PERIODIC_WAKEUP;                    // Periodic wake-up [131:131]
+  UINT8   TCASE_SUPPORT;                      // Package case temperature is controlled [132:132]
+  UINT8   PRODUCTION_STATE_AWARENESS;         // Production state awareness [133:133]
+  UINT8   SEC_BAD_BLK_MGMNT;                  // Bad block management mode [134:134]
+  UINT8   RESERVED_5;                         // Reserved [135:135]
+  UINT8   ENH_START_ADDR[4];                  // Enhanced user data start address [139:136]
+  UINT8   ENH_SIZE_MULT[3];                   // Enhanced user data area size [142:140]
+  UINT8   GP_SIZE_MULT[12];                   // General purpose partition size [154:143]
+  UINT8   PARTITION_SETTING_COMPLETED;        // Partitioning setting [155:155]
+  UINT8   PARTITIONS_ATTRIBUTE;               // Partitions attribute [156:156]
+  UINT8   MAX_ENH_SIZE_MULT[3];               // Max enhanced area size [159:157]
+  UINT8   PARTITIONING_SUPPORT;               // Partitioning [160:160]
+  UINT8   HPI_MGMT;                           // HPI management [161:161]
+  UINT8   RST_N_FUNCTION;                     // H/W reset function [162:162]
+  UINT8   BKOPS_EN;                           // Enable background operations handshake [163:163]
+  UINT8   BKOPS_START;                        // Manually start background operations [164:164]
+  UINT8   SANITIZE_START;                     // Start sanitize operation [165:165]
+  UINT8   WR_REL_PARAM;                       // Write reliability parameter register [166:166]
+  UINT8   WR_REL_SET;                         // Write reliability setting register [167:167]
+  UINT8   RPMB_SIZE_MULT;                     // RPMB size [168:168]
+  UINT8   FW_CONFIG;                          // FW configuration [169:169]
+  UINT8   RESERVED_6;                         // Reserved [170:170]
+  UINT8   USER_WP;                            // User area write protection register [171:171]
+  UINT8   RESERVED_7;                         // Reserved [172:172]
+  UINT8   BOOT_WP;                            // Boot area write protection register [173:173]
+  UINT8   BOOT_WP_STATUS;                     // Boot write protection register [174:174]
+  UINT8   ERASE_GROUP_DEF;                    // High-density erase group definition [175:175]
+  UINT8   RESERVED_8;                         // Reserved [176:176]
+  UINT8   BOOT_BUS_CONDITIONS;                // Boot bus conditions [177:177]
+  UINT8   BOOT_CONFIG_PROT;                   // Boot config protection [178:178]
+  UINT8   PARTITION_CONFIG;                   // Partition config [179:179]
+  UINT8   RESERVED_9;                         // Reserved [180:180]
+  UINT8   ERASED_MEM_CONT;                    // Erased memory content [181:181]
+  UINT8   RESERVED_10;                        // Reserved [182:182]
+  UINT8   BUS_WIDTH;                          // Bus width mode [183:183]
+  UINT8   RESERVED_11;                        // Reserved [184:184]
+  UINT8   HS_TIMING;                          // High-speed interface timing [185:185]
+  UINT8   RESERVED_12;                        // Reserved [186:186]
+  UINT8   POWER_CLASS;                        // Power class [187:187]
+  UINT8   RESERVED_13;                        // Reserved [188:188]
+  UINT8   CMD_SET_REV;                        // Command set revision [189:189]
+  UINT8   RESERVED_14;                        // Reserved [190:190]
+  UINT8   CMD_SET;                            // Command set [191:191]
+  UINT8   EXT_CSD_REV;                        // Extended CSD revision [192:192]
+  UINT8   RESERVED_15;                        // Reserved [193:193]
+  UINT8   CSD_STRUCTURE;                      // CSD Structure [194:194]
+  UINT8   RESERVED_16;                        // Reserved [195:195]
+  UINT8   DEVICE_TYPE;                        // Device type [196:196]
+  UINT8   DRIVER_STRENGTH;                    // I/O Driver strength [197:197]
+  UINT8   OUT_OF_INTERRUPT_TIME;              // Out-of-interrupt busy timing [198:198]
+  UINT8   PARTITION_SWITCH_TIME;              // Partition switching timing [199:199]
+  UINT8   PWR_CL_52_195;                      // Power class for 52MHz at 1.95V 1 R [200:200]
+  UINT8   PWR_CL_26_195;                      // Power class for 26MHz at 1.95V 1 R [201:201]
+  UINT8   PWR_CL_52_360;                      // Power class for 52MHz at 3.6V 1 R [202:202]
+  UINT8   PWR_CL_26_360;                      // Power class for 26MHz at 3.6V 1 R [203:203]
+  UINT8   RESERVED_17;                        // Reserved [204:204]
+  UINT8   MIN_PERF_R_4_26;                    // Minimum read performance for 4bit at 26MHz [205:205]
+  UINT8   MIN_PERF_W_4_26;                    // Minimum write performance for 4bit at 26MHz [206:206]
+  UINT8   MIN_PERF_R_8_26_4_52;               // Minimum read performance for 8bit at 26MHz, for 4bit at 52MHz [207:207]
+  UINT8   MIN_PERF_W_8_26_4_52;               // Minimum write performance for 8bit at 26MHz, for 4bit at 52MHz [208:208]
+  UINT8   MIN_PERF_R_8_52;                    // Minimum read performance for 8bit at 52MHz [209:209]
+  UINT8   MIN_PERF_W_8_52;                    // Minimum write performance for 8bit at 52MHz [210:210]
+  UINT8   RESERVED_18;                        // Reserved [211:211]
+  UINT32  SEC_COUNT;                          // Sector count [215:212]
+  UINT8   SLEEP_NOTIFICATION_TIME;            // Sleep notification timout [216:216]
+  UINT8   S_A_TIMEOUT;                        // Sleep/awake timeout [217:217]
+  UINT8   PRODUCTION_STATE_AWARENESS_TIMEOUT; // Production state awareness timeout [218:218]
+  UINT8   S_C_VCCQ;                           // Sleep current (VCCQ) [219:219]
+  UINT8   S_C_VCC;                            // Sleep current (VCC) [220:220]
+  UINT8   HC_WP_GRP_SIZE;                     // High-capacity write protect group size [221:221]
+  UINT8   REL_WR_SEC_C;                       // Reliable write sector count [222:222]
+  UINT8   ERASE_TIMEOUT_MULT;                 // High-capacity erase timeout [223:223]
+  UINT8   HC_ERASE_GRP_SIZE;                  // High-capacity erase unit size [224:224]
+  UINT8   ACC_SIZE;                           // Access size [225:225]
+  UINT8   BOOT_SIZE_MULTI;                    // Boot partition size [226:226]
+  UINT8   RESERVED_19;                        // Reserved [227:227]
+  UINT8   BOOT_INFO;                          // Boot information [228:228]
+  UINT8   SEC_TRIM_MULT;                      // Secure TRIM Multiplier [229:229]
+  UINT8   SEC_ERASE_MULT;                     // Secure Erase Multiplier [230:230]
+  UINT8   SEC_FEATURE_SUPPORT;                // Secure Feature Support [231:231]
+  UINT8   TRIM_MULT;                          // TRIM Multiplier [232:232]
+  UINT8   RESERVED_20;                        // Reserved [233:233]
+  UINT8   MIN_PREF_DDR_R_8_52;                // Minimum read performance for 8bit at 52MHz in DDR mode [234:234]
+  UINT8   MIN_PREF_DDR_W_8_52;                // Minimum write performance for 8bit at 52MHz in DDR mode [235:235]
+  UINT8   PWR_CL_200_130;                     // Power class for 200MHz at VCCQ=1.3V, VCC=3.6V [236:236]
+  UINT8   PWR_CL_200_195;                     // Power class for 200MHz at VCCQ=1.95V, VCC=3.6V [237:237]
+  UINT8   PWR_CL_DDR_52_195;                  // Power class for 52MHz, DDR at 1.95V [238:238]
+  UINT8   PWR_CL_DDR_52_360;                  // Power class for 52Mhz, DDR at 3.6V [239:239]
+  UINT8   RESERVED_21;                        // Reserved [240:240]
+  UINT8   INI_TIMEOUT_AP;                     // 1st initialization time after partitioning [241:241]
+  UINT8   CORRECTLY_PRG_SECTORS_NUM[4];       // Number of correctly programmed sectors [245:242]
+  UINT8   BKOPS_STATUS;                       // Background operations status [246:246]
+  UINT8   POWER_OFF_LONG_TIME;                // Power off notification (long) timeout [247:247]
+  UINT8   GENERIC_CMD6_TIME;                  // Generic CMD6 timeout [248:248]
+  UINT8   CACHE_SIZE[4];                      // Cache size [252:249]
+  UINT8   PWR_CL_DDR_200_360;                 // Power class for 200MHz, DDR at VCC=3.6V [253:253]
+  UINT8   FIRMWARE_VERSION[8];                // Firmware version [261:254]
+  UINT8   DEVICE_VERSION[2];                  // Device version [263:262]
+  UINT8   OPTIMAL_TRIM_UNIT_SIZE;             // Optimal trim unit size [264:264]
+  UINT8   OPTIMAL_WRITE_SIZE;                 // Optimal write size [265:265]
+  UINT8   OPTIMAL_READ_SIZE;                  // Optimal read size [266:266]
+  UINT8   PRE_EOL_INFO;                       // Pre EOL information [267:267]
+  UINT8   DEVICE_LIFE_TIME_EST_TYP_A;         // Device life time estimation type A [268:268]
+  UINT8   DEVICE_LIFE_TIME_EST_TYP_B;         // Device life time estimation type B [269:269]
+  UINT8   VENDOR_PROPRIETARY_HEALTH_REPORT[32];         // Vendor proprietary health report [301:270]
+  UINT8   NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED[4]; // Number of FW sectors correctly programmed [305:302]
+  UINT8   RESERVED_22[181];                   // Reserved [486:306]
+  UINT8   FFU_ARG[4];                         // FFU argument [490:487]
+  UINT8   OPERATION_CODE_TIMEOUT;             // Operation codes timeout [491:491]
+  UINT8   FFU_FEATURES;                       // FFU features [492:492]
+  UINT8   SUPPORTED_MODES;                    // Supported modes [493:493]
+  UINT8   EXT_SUPPORT;                        // Extended partitions attribute support [494:494]
+  UINT8   LARGE_UNIT_SIZE_M1;                 // Large unit size [495:495]
+  UINT8   CONTEXT_CAPABILITIES;               // Context management capabilities [496:496]
+  UINT8   TAG_RES_SIZE;                       // Tag resource size [497:497]
+  UINT8   TAG_UNIT_SIZE;                      // Tag unit size [498:498]
+  UINT8   DATA_TAG_SUPPORT;                   // Data tag support [499:499]
+  UINT8   MAX_PACKED_WRITES;                  // Max packed write commands [500:500]
+  UINT8   MAX_PACKED_READS;                   // Max packed read commands [501:501]
+  UINT8   BKOPS_SUPPORT;                      // Background operations support [502:502]
+  UINT8   HPI_FEATURES;                       // HPI features [503:503]
+  UINT8   S_CMD_SET;                          // Supported command sets [504:504]
+  UINT8   EXT_SECURITY_ERR;                   // Extended security commands error [505:505]
+  UINT8   RESERVED_23[6];                     // Reserved [511:506]
+} ECSD;
+
 typedef struct  {
   UINT16    RCA;
   CARD_TYPE CardType;
   OCR       OCRData;
   CID       CIDData;
   CSD       CSDData;
-  UINT32    ECSD[128];      // MMC V4 extended card specific
+  ECSD      ECSDData;                         // MMC V4 extended card specific
 } CARD_INFO;
 
 typedef struct _MMC_HOST_INSTANCE {
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 578fcb6..c5897de 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -90,7 +90,7 @@ EmmcIdentificationMode (
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
   }
 
-  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSD));
+  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
     return Status;
@@ -103,7 +103,7 @@ EmmcIdentificationMode (
   Media->LogicalBlocksPerPhysicalBlock = 1;
   Media->IoAlign = 4;
   // Compute last block using bits [215:212] of the ECSD
-  Media->LastBlock = MmcHostInstance->CardInfo.ECSD[EMMC_ECSD_SIZE_OFFSET] - 1; // eMMC isn't supposed to report this for
+  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SEC_COUNT - 1; // eMMC isn't supposed to report this for
   // Cards <2GB in size, but the model does.
 
   // Setup card type
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 06/11] MmcDxe: set io bus width before reading EXTCSD
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (4 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 05/11] MmcDxe: declare ECSD structure Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD Haojian Zhuang
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Set io bus width on both MMC controller and EXTCSD. Otherwise, it may
cause unmatched failure case. And support more timing mode, high speed,
HS200 & HS400 mode.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Include/Protocol/MmcHost.h           |  16 +-
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 229 ++++++++++++++++++++---
 2 files changed, 221 insertions(+), 24 deletions(-)

diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
index a242291..01c0bf4 100644
--- a/EmbeddedPkg/Include/Protocol/MmcHost.h
+++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
@@ -49,6 +49,7 @@ typedef UINT32 MMC_CMD;
 #define MMC_CMD2              (MMC_INDX(2) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_LONG_RESPONSE)
 #define MMC_CMD3              (MMC_INDX(3) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD5              (MMC_INDX(5) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE)
+#define MMC_CMD6              (MMC_INDX(6) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD7              (MMC_INDX(7) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD8              (MMC_INDX(8) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD9              (MMC_INDX(9) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_LONG_RESPONSE)
@@ -82,6 +83,16 @@ typedef enum _MMC_STATE {
     MmcDisconnectState,
 } MMC_STATE;
 
+#define EMMCBACKWARD         (0)
+#define EMMCHS26             (1 << 0)      // High-Speed @26MHz at rated device voltages
+#define EMMCHS52             (1 << 1)      // High-Speed @52MHz at rated device voltages
+#define EMMCHS52DDR1V8       (1 << 2)      // High-Speed Dual Data Rate @52MHz 1.8V or 3V I/O
+#define EMMCHS52DDR1V2       (1 << 3)      // High-Speed Dual Data Rate @52MHz 1.2V I/O
+#define EMMCHS200SDR1V8      (1 << 4)      // HS200 Single Data Rate @200MHz 1.8V I/O
+#define EMMCHS200SDR1V2      (1 << 5)      // HS200 Single Data Rate @200MHz 1.2V I/O
+#define EMMCHS400DDR1V8      (1 << 6)      // HS400 Dual Data Rate @400MHz 1.8V I/O
+#define EMMCHS400DDR1V2      (1 << 7)      // HS400 Dual Data Rate @400MHz 1.2V I/O
+
 ///
 /// Forward declaration for EFI_MMC_HOST_PROTOCOL
 ///
@@ -133,8 +144,9 @@ typedef EFI_STATUS (EFIAPI *MMC_WRITEBLOCKDATA) (
 
 typedef EFI_STATUS (EFIAPI *MMC_SETIOS) (
   IN  EFI_MMC_HOST_PROTOCOL     *This,
-  IN  UINT32                    BusClockRate,
-  IN  UINT32                    BusWidth
+  IN  UINT32                    BusClockFreq,
+  IN  UINT32                    BusWidth,
+  IN  UINT32                    TimingMode
   );
 
 
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index c5897de..cefc2b6 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -25,11 +25,109 @@ typedef union {
 #define EMMC_CARD_SIZE          512
 #define EMMC_ECSD_SIZE_OFFSET   53
 
+#define EXTCSD_BUS_WIDTH        183
+#define EXTCSD_HS_TIMING        185
+
+#define EMMC_TIMING_BACKWARD    0
+#define EMMC_TIMING_HS          1
+#define EMMC_TIMING_HS200       2
+#define EMMC_TIMING_HS400       3
+
+#define EMMC_BUS_WIDTH_1BIT     0
+#define EMMC_BUS_WIDTH_4BIT     1
+#define EMMC_BUS_WIDTH_8BIT     2
+#define EMMC_BUS_WIDTH_DDR_4BIT 5
+#define EMMC_BUS_WIDTH_DDR_8BIT 6
+
+#define EMMC_SWITCH_ERROR       (1 << 7)
+
+#define DEVICE_STATE(x)         (((x) >> 9) & 0xf)
+typedef enum _EMMC_DEVICE_STATE {
+  EMMC_IDLE_STATE = 0,
+  EMMC_READY_STATE,
+  EMMC_IDENT_STATE,
+  EMMC_STBY_STATE,
+  EMMC_TRAN_STATE,
+  EMMC_DATA_STATE,
+  EMMC_RCV_STATE,
+  EMMC_PRG_STATE,
+  EMMC_DIS_STATE,
+  EMMC_BTST_STATE,
+  EMMC_SLP_STATE
+} EMMC_DEVICE_STATE;
+
 UINT32 mEmmcRcaCount = 0;
 
 STATIC
 EFI_STATUS
 EFIAPI
+EmmcGetDeviceState (
+  IN  MMC_HOST_INSTANCE    *MmcHostInstance,
+  OUT EMMC_DEVICE_STATE    *State
+  )
+{
+  EFI_MMC_HOST_PROTOCOL *Host;
+  EFI_STATUS Status;
+  UINT32     Data, RCA;
+
+  if (State == NULL)
+    return EFI_INVALID_PARAMETER;
+
+  Host  = MmcHostInstance->MmcHost;
+  RCA = MmcHostInstance->CardInfo.RCA << RCA_SHIFT_OFFSET;
+  Status = Host->SendCommand (Host, MMC_CMD13, RCA);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get card status, Status=%r.\n", Status));
+    return Status;
+  }
+  Status = Host->ReceiveResponse (Host, MMC_RESPONSE_TYPE_R1, &Data);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get response of CMD13, Status=%r.\n", Status));
+    return Status;
+  }
+  if (Data & EMMC_SWITCH_ERROR) {
+    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to switch expected mode, Status=%r.\n", Status));
+    return EFI_DEVICE_ERROR;
+  }
+  *State = DEVICE_STATE(Data);
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+EmmcSetEXTCSD (
+  IN MMC_HOST_INSTANCE     *MmcHostInstance,
+  UINT32                   ExtCmdIndex,
+  UINT32                   Value
+  )
+{
+  EFI_MMC_HOST_PROTOCOL *Host;
+  EMMC_DEVICE_STATE     State;
+  EFI_STATUS Status;
+  UINT32     Argument;
+
+  Host  = MmcHostInstance->MmcHost;
+  Argument = (3 << 24) | ((ExtCmdIndex & 0xff) << 16) | ((Value & 0xff) << 8) | 1;
+  Status = Host->SendCommand (Host, MMC_CMD6, Argument);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "EmmcSetEXTCSD(): Failed to send CMD6, Status=%r.\n", Status));
+    return Status;
+  }
+  // Make sure device exiting prog mode
+  do {
+    Status = EmmcGetDeviceState (MmcHostInstance, &State);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "EmmcSetEXTCSD(): Failed to get device state, Status=%r.\n", Status));
+      return Status;
+    }
+  } while (State == EMMC_PRG_STATE);
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
 EmmcIdentificationMode (
   IN MMC_HOST_INSTANCE     *MmcHostInstance,
   IN OCR_RESPONSE           Response
@@ -37,6 +135,7 @@ EmmcIdentificationMode (
 {
   EFI_MMC_HOST_PROTOCOL *Host;
   EFI_BLOCK_IO_MEDIA    *Media;
+  EMMC_DEVICE_STATE     State;
   EFI_STATUS Status;
   UINT32     RCA;
 
@@ -84,35 +183,117 @@ EmmcIdentificationMode (
     DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Card selection error, Status=%r.\n", Status));
   }
 
-  // Fetch ECSD
-  Status = Host->SendCommand (Host, MMC_CMD8, RCA);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
-  }
-
-  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
-    return Status;
-  }
-
+  // MMC v4 specific
+  if (MmcHostInstance->CardInfo.CSDData.SPEC_VERS == 4) {
+    if (Host->SetIos) {
+      // Set 1-bit bus width
+      Status = Host->SetIos (Host, 0, 1, EMMCBACKWARD);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Set 1-bit bus width error, Status=%r.\n", Status));
+        return Status;
+      }
+
+      // Set 1-bit bus width for EXTCSD
+      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_1BIT);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Set extcsd bus width error, Status=%r.\n", Status));
+        return Status;
+      }
+    }
+
+    // Fetch ECSD
+    Status = Host->SendCommand (Host, MMC_CMD8, RCA);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
+    }
+    Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
+      return Status;
+    }
+
+    // Make sure device exiting data mode
+    do {
+      Status = EmmcGetDeviceState (MmcHostInstance, &State);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Failed to get device state, Status=%r.\n", Status));
+        return Status;
+      }
+    } while (State == EMMC_DATA_STATE);
+
+    // Compute last block using bits [215:212] of the ECSD
+    Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SEC_COUNT - 1; // eMMC isn't supposed to report this for
+    // Cards <2GB in size, but the model does.
+
+    // Setup card type
+    MmcHostInstance->CardInfo.CardType = EMMC_CARD;
+  }
   // Set up media
   Media->BlockSize = EMMC_CARD_SIZE; // 512-byte support is mandatory for eMMC cards
   Media->MediaId = MmcHostInstance->CardInfo.CIDData.PSN;
   Media->ReadOnly = MmcHostInstance->CardInfo.CSDData.PERM_WRITE_PROTECT;
   Media->LogicalBlocksPerPhysicalBlock = 1;
   Media->IoAlign = 4;
-  // Compute last block using bits [215:212] of the ECSD
-  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SEC_COUNT - 1; // eMMC isn't supposed to report this for
-  // Cards <2GB in size, but the model does.
-
-  // Setup card type
-  MmcHostInstance->CardInfo.CardType = EMMC_CARD;
   return EFI_SUCCESS;
 }
 
 STATIC
 EFI_STATUS
+InitializeEmmcDevice (
+  IN  MMC_HOST_INSTANCE   *MmcHostInstance
+  )
+{
+  EFI_MMC_HOST_PROTOCOL *Host;
+  EFI_STATUS Status = EFI_SUCCESS;
+  ECSD       *ECSDData;
+  BOOLEAN    Found = FALSE;
+  UINT32     BusClockFreq, Idx;
+  UINT32     TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, EMMCHS26};
+
+  Host  = MmcHostInstance->MmcHost;
+  if (MmcHostInstance->CardInfo.CSDData.SPEC_VERS < 4)
+    return EFI_SUCCESS;
+  ECSDData = &MmcHostInstance->CardInfo.ECSDData;
+  if (ECSDData->DEVICE_TYPE == EMMCBACKWARD)
+    return EFI_SUCCESS;
+
+  if (Host->SetIos) {
+    Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_HS_TIMING, EMMC_TIMING_HS);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to switch high speed mode, Status:%r.\n", Status));
+      return Status;
+    }
+
+    for (Idx = 0; Idx < 4; Idx++) {
+      switch (TimingMode[Idx]) {
+      case EMMCHS52DDR1V2:
+      case EMMCHS52DDR1V8:
+      case EMMCHS52:
+        BusClockFreq = 52000000;
+        break;
+      case EMMCHS26:
+        BusClockFreq = 26000000;
+        break;
+      default:
+        return EFI_UNSUPPORTED;
+      }
+      Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
+      if (!EFI_ERROR (Status)) {
+        Found = TRUE;
+        break;
+      }
+    }
+    if (Found) {
+      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
+      if (EFI_ERROR (Status))
+        DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
+    }
+  }
+  return Status;
+}
+
+STATIC
+EFI_STATUS
 InitializeSdMmcDevice (
   IN  MMC_HOST_INSTANCE   *MmcHostInstance
   )
@@ -429,11 +610,15 @@ InitializeMmcDevice (
     return Status;
   }
 
-  if (MmcHostInstance->CardInfo.CardType != EMMC_CARD) {
-    Status = InitializeSdMmcDevice (MmcHostInstance);
-    if (EFI_ERROR (Status)) {
-      return Status;
+  if (MmcHostInstance->CardInfo.CardType == EMMC_CARD) {
+    if (MmcHost->SetIos) {
+      Status = InitializeEmmcDevice (MmcHostInstance);
     }
+  } else {
+    Status = InitializeSdMmcDevice (MmcHostInstance);
+  }
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
   // Set Block Length
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (5 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 06/11] MmcDxe: set io bus width before reading EXTCSD Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 23:59   ` Leif Lindholm
  2016-11-08 15:21 ` [PATCH v4 08/11] MmcDxe: set iospeed and bus width in SD stack Haojian Zhuang
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

When SD card is used, mediaid is not initialized and used directly. So
fix it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index cefc2b6..5b802c0 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -57,6 +57,7 @@ typedef enum _EMMC_DEVICE_STATE {
 } EMMC_DEVICE_STATE;
 
 UINT32 mEmmcRcaCount = 0;
+UINT32 CurrentMediaId = 0;
 
 STATIC
 EFI_STATUS
@@ -231,6 +232,10 @@ EmmcIdentificationMode (
   // Set up media
   Media->BlockSize = EMMC_CARD_SIZE; // 512-byte support is mandatory for eMMC cards
   Media->MediaId = MmcHostInstance->CardInfo.CIDData.PSN;
+  if (CurrentMediaId > Media->MediaId)
+    Media->MediaId = ++CurrentMediaId;
+  else
+    CurrentMediaId = Media->MediaId;
   Media->ReadOnly = MmcHostInstance->CardInfo.CSDData.PERM_WRITE_PROTECT;
   Media->LogicalBlocksPerPhysicalBlock = 1;
   Media->IoAlign = 4;
@@ -344,7 +349,7 @@ InitializeSdMmcDevice (
   MmcHostInstance->BlockIo.Media->BlockSize    = BlockSize;
   MmcHostInstance->BlockIo.Media->ReadOnly     = MmcHost->IsReadOnly (MmcHost);
   MmcHostInstance->BlockIo.Media->MediaPresent = TRUE;
-  MmcHostInstance->BlockIo.Media->MediaId++;
+  MmcHostInstance->BlockIo.Media->MediaId      = ++CurrentMediaId;
 
   CmdArg = MmcHostInstance->CardInfo.RCA << 16;
   Status = MmcHost->SendCommand (MmcHost, MMC_CMD7, CmdArg);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 08/11] MmcDxe: set iospeed and bus width in SD stack
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (6 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 09/11] MmcDxe: expand to support multiple blocks Haojian Zhuang
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Add more SD commands to support 4-bit bus width & iospeed. It's not
formal code. And it needs to be updated later.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Include/Protocol/MmcHost.h           |   3 +
 EmbeddedPkg/Universal/MmcDxe/Mmc.h               |  17 +++
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 138 ++++++++++++++++++++---
 3 files changed, 142 insertions(+), 16 deletions(-)

diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
index 01c0bf4..5028045 100644
--- a/EmbeddedPkg/Include/Protocol/MmcHost.h
+++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
@@ -64,11 +64,14 @@ typedef UINT32 MMC_CMD;
 #define MMC_CMD24             (MMC_INDX(24) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD55             (MMC_INDX(55) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_ACMD41            (MMC_INDX(41) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE)
+#define MMC_ACMD51            (MMC_INDX(51) | MMC_CMD_WAIT_RESPONSE)
 
 // Valid responses for CMD1 in eMMC
 #define EMMC_CMD1_CAPACITY_LESS_THAN_2GB 0x00FF8080 // Capacity <= 2GB, byte addressing used
 #define EMMC_CMD1_CAPACITY_GREATER_THAN_2GB 0x40FF8080 // Capacity > 2GB, 512-byte sector addressing used
 
+#define MMC_STATUS_APP_CMD    (1 << 5)
+
 typedef enum _MMC_STATE {
     MmcInvalidState = 0,
     MmcHwInitializationState,
diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
index d3e9b71..6f735c4 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
@@ -75,6 +75,23 @@ typedef struct {
   UINT32  PowerUp:     1; // This bit is set to LOW if the card has not finished the power up routine
 } OCR;
 
+/* For little endian CPU */
+typedef struct {
+  UINT8   SD_SPEC:               4; // SD Memory Card - Spec. Version [59:56]
+  UINT8   SCR_STRUCTURE:         4; // SCR Structure [63:60]
+  UINT8   SD_BUS_WIDTHS:         4; // DAT Bus widths supported [51:48]
+  UINT8   DATA_STAT_AFTER_ERASE: 1; // Data Status after erases [55]
+  UINT8   SD_SECURITY:           3; // CPRM Security Support [54:52]
+  UINT8   EX_SECURITY_1:         1; // Extended Security Support [43]
+  UINT8   SD_SPEC4:              1; // Spec. Version 4.00 or higher [42]
+  UINT8   RESERVED_1:            2; // Reserved [41:40]
+  UINT8   SD_SPEC3:              1; // Spec. Version 3.00 or higher [47]
+  UINT8   EX_SECURITY_2:         3; // Extended Security Support [46:44]
+  UINT8   CMD_SUPPORT:           4; // Command Support bits [35:32]
+  UINT8   RESERVED_2:            4; // Reserved [39:36]
+  UINT32  RESERVED_3;               // Manufacturer Usage [31:0]
+} SCR;
+
 typedef struct {
   UINT32  NOT_USED;   // 1 [0:0]
   UINT32  CRC;        // CRC7 checksum [7:1]
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 5b802c0..6fb09e6 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -12,6 +12,9 @@
 *
 **/
 
+#include <Library/BaseMemoryLib.h>
+#include <Library/TimerLib.h>
+
 #include "Mmc.h"
 
 typedef union {
@@ -41,6 +44,11 @@ typedef union {
 
 #define EMMC_SWITCH_ERROR       (1 << 7)
 
+#define SD_BUS_WIDTH_1BIT       (1 << 0)
+#define SD_BUS_WIDTH_4BIT       (1 << 2)
+
+#define SD_CCC_SWITCH           (1 << 10)
+
 #define DEVICE_STATE(x)         (((x) >> 9) & 0xf)
 typedef enum _EMMC_DEVICE_STATE {
   EMMC_IDLE_STATE = 0,
@@ -69,28 +77,30 @@ EmmcGetDeviceState (
 {
   EFI_MMC_HOST_PROTOCOL *Host;
   EFI_STATUS Status;
-  UINT32     Data, RCA;
+  UINT32     Rsp[4], RCA;
 
   if (State == NULL)
     return EFI_INVALID_PARAMETER;
 
   Host  = MmcHostInstance->MmcHost;
   RCA = MmcHostInstance->CardInfo.RCA << RCA_SHIFT_OFFSET;
-  Status = Host->SendCommand (Host, MMC_CMD13, RCA);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get card status, Status=%r.\n", Status));
-    return Status;
-  }
-  Status = Host->ReceiveResponse (Host, MMC_RESPONSE_TYPE_R1, &Data);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get response of CMD13, Status=%r.\n", Status));
-    return Status;
-  }
-  if (Data & EMMC_SWITCH_ERROR) {
-    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to switch expected mode, Status=%r.\n", Status));
-    return EFI_DEVICE_ERROR;
-  }
-  *State = DEVICE_STATE(Data);
+  do {
+    Status = Host->SendCommand (Host, MMC_CMD13, RCA);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get card status, Status=%r.\n", Status));
+      return Status;
+    }
+    Status = Host->ReceiveResponse (Host, MMC_RESPONSE_TYPE_R1, Rsp);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get response of CMD13, Status=%r.\n", Status));
+      return Status;
+    }
+    if (Rsp[0] & EMMC_SWITCH_ERROR) {
+      DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to switch expected mode, Status=%r.\n", Status));
+      return EFI_DEVICE_ERROR;
+    }
+  } while (!(Rsp[0] & MMC_R0_READY_FOR_DATA));
+  *State = MMC_R0_CURRENTSTATE(Rsp);
   return EFI_SUCCESS;
 }
 
@@ -305,9 +315,12 @@ InitializeSdMmcDevice (
 {
   UINT32        CmdArg;
   UINT32        Response[4];
+  UINT32        Buffer[128];
   UINTN         BlockSize;
   UINTN         CardSize;
   UINTN         NumBlocks;
+  BOOLEAN       CccSwitch;
+  SCR           Scr;
   EFI_STATUS    Status;
   EFI_MMC_HOST_PROTOCOL     *MmcHost;
 
@@ -328,6 +341,10 @@ InitializeSdMmcDevice (
     return Status;
   }
   PrintCSD (Response);
+  if (MMC_CSD_GET_CCC(Response) & SD_CCC_SWITCH)
+    CccSwitch = TRUE;
+  else
+    CccSwitch = FALSE;
 
   if (MmcHostInstance->CardInfo.CardType == SD_CARD_2_HIGH) {
     CardSize = HC_MMC_CSD_GET_DEVICESIZE (Response);
@@ -358,6 +375,95 @@ InitializeSdMmcDevice (
     return Status;
   }
 
+  Status = MmcHost->SendCommand (MmcHost, MMC_CMD55, CmdArg);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status));
+    return Status;
+  }
+  Status = MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status));
+    return Status;
+  }
+  if ((Response[0] & MMC_STATUS_APP_CMD) == 0)
+    return EFI_SUCCESS;
+
+  /* SCR */
+  Status = MmcHost->SendCommand (MmcHost, MMC_ACMD51, 0);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "%a(MMC_ACMD51): Error and Status = %r\n", __func__, Status));
+    return Status;
+  } else {
+    Status = MmcHost->ReadBlockData (MmcHost, 0, 8, Buffer);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "%a(MMC_ACMD51): ReadBlockData Error and Status = %r\n", __func__, Status));
+      return Status;
+    }
+    CopyMem (&Scr, Buffer, 8);
+    if (Scr.SD_SPEC == 2) {
+      if (Scr.SD_SPEC3 == 1) {
+	if (Scr.SD_SPEC4 == 1) {
+          DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 4.xx\n"));
+	} else {
+          DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 3.0x\n"));
+	}
+      } else {
+	if (Scr.SD_SPEC4 == 0) {
+          DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 2.0\n"));
+	} else {
+	  DEBUG ((EFI_D_ERROR, "Found invalid SD Card\n"));
+	}
+      }
+    } else {
+      if ((Scr.SD_SPEC3 == 0) && (Scr.SD_SPEC4 == 0)) {
+        if (Scr.SD_SPEC == 1) {
+	  DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 1.10\n"));
+	} else {
+	  DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 1.0\n"));
+	}
+      } else {
+        DEBUG ((EFI_D_ERROR, "Found invalid SD Card\n"));
+      }
+    }
+  }
+  if (CccSwitch) {
+    /* SD Switch, Mode:1, Group:0, Value:1 */
+    CmdArg = 1 << 31 | 0x00FFFFFF;
+    CmdArg &= ~(0xF << (0 * 4));
+    CmdArg |= 1 << (0 * 4);
+    Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, CmdArg);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status));
+       return Status;
+    } else {
+      Status = MmcHost->ReadBlockData (MmcHost, 0, 64, Buffer);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Error and Status = %r\n", Status));
+        return Status;
+      }
+    }
+  }
+  if (Scr.SD_BUS_WIDTHS & SD_BUS_WIDTH_4BIT) {
+    CmdArg = MmcHostInstance->CardInfo.RCA << 16;
+    Status = MmcHost->SendCommand (MmcHost, MMC_CMD55, CmdArg);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status));
+      return Status;
+    }
+    /* Width: 4 */
+    Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, 2);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status));
+      return Status;
+    }
+  }
+  if (MmcHost->SetIos) {
+    Status = MmcHost->SetIos (MmcHost, 24 * 1000 * 1000, 4, EMMCBACKWARD);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "%a(SetIos): Error and Status = %r\n", Status));
+      return Status;
+    }
+  }
   return EFI_SUCCESS;
 }
 
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 09/11] MmcDxe: expand to support multiple blocks
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (7 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 08/11] MmcDxe: set iospeed and bus width in SD stack Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-09  0:14   ` Leif Lindholm
  2016-11-08 15:21 ` [PATCH v4 10/11] PL180: update for indentifying SD Haojian Zhuang
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Make use of DMA to transfer multiple blocks at one time. It could
improve the performance on MMC/SD driver.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 EmbeddedPkg/Include/Protocol/MmcHost.h    |   6 +
 EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 175 ++++++++++++++++++++----------
 2 files changed, 122 insertions(+), 59 deletions(-)

diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
index 5028045..cd5335a 100644
--- a/EmbeddedPkg/Include/Protocol/MmcHost.h
+++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
@@ -62,6 +62,7 @@ typedef UINT32 MMC_CMD;
 #define MMC_CMD20             (MMC_INDX(20) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD23             (MMC_INDX(23) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD24             (MMC_INDX(24) | MMC_CMD_WAIT_RESPONSE)
+#define MMC_CMD25             (MMC_INDX(25) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_CMD55             (MMC_INDX(55) | MMC_CMD_WAIT_RESPONSE)
 #define MMC_ACMD41            (MMC_INDX(41) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE)
 #define MMC_ACMD51            (MMC_INDX(51) | MMC_CMD_WAIT_RESPONSE)
@@ -152,6 +153,10 @@ typedef EFI_STATUS (EFIAPI *MMC_SETIOS) (
   IN  UINT32                    TimingMode
   );
 
+typedef BOOLEAN (EFIAPI *MMC_ISMULTIBLOCK) (
+  IN  EFI_MMC_HOST_PROTOCOL     *This
+  );
+
 
 struct _EFI_MMC_HOST_PROTOCOL {
 
@@ -169,6 +174,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
   MMC_WRITEBLOCKDATA      WriteBlockData;
 
   MMC_SETIOS              SetIos;
+  MMC_ISMULTIBLOCK        IsMultiBlock;
 
 };
 
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
index 0e1ef57..043ca92 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
@@ -126,6 +126,95 @@ MmcStopTransmission (
 #define MMCI0_BLOCKLEN 512
 #define MMCI0_TIMEOUT  10000
 
+STATIC EFI_STATUS
+MmcTransferBlock (
+  IN EFI_BLOCK_IO_PROTOCOL    *This,
+  IN UINTN                    Cmd,
+  IN UINTN                    Transfer,
+  IN UINT32                   MediaId,
+  IN EFI_LBA                  Lba,
+  IN UINTN                    BufferSize,
+  OUT VOID                    *Buffer
+  )
+{
+  EFI_STATUS              Status;
+  UINTN                   CmdArg;
+  INTN                    Timeout;
+  UINT32                  Response[4];
+  MMC_HOST_INSTANCE       *MmcHostInstance;
+  EFI_MMC_HOST_PROTOCOL   *MmcHost;
+
+  MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS (This);
+  MmcHost = MmcHostInstance->MmcHost;
+
+  //Set command argument based on the card access mode (Byte mode or Block mode)
+  if (MmcHostInstance->CardInfo.OCRData.AccessMode & BIT1) {
+    CmdArg = Lba;
+  } else {
+    CmdArg = Lba * This->Media->BlockSize;
+  }
+
+  Status = MmcHost->SendCommand (MmcHost, Cmd, CmdArg);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "%a(MMC_CMD%d): Error %r\n", __func__, Cmd, Status));
+    return Status;
+  }
+
+  if (Transfer == MMC_IOBLOCKS_READ) {
+    // Read Data
+    Status = MmcHost->ReadBlockData (MmcHost, Lba, BufferSize, Buffer);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_BLKIO, "%a(): Error Read Block Data and Status = %r\n", __func__, Status));
+      MmcStopTransmission (MmcHost);
+      return Status;
+    }
+    Status = MmcNotifyState (MmcHostInstance, MmcProgrammingState);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "%a() : Error MmcProgrammingState\n", __func__));
+      return Status;
+    }
+  } else {
+    // Write Data
+    Status = MmcHost->WriteBlockData (MmcHost, Lba, BufferSize, Buffer);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_BLKIO, "%a(): Error Write Block Data and Status = %r\n", __func__, Status));
+      MmcStopTransmission (MmcHost);
+      return Status;
+    }
+  }
+
+  // Command 13 - Read status and wait for programming to complete (return to tran)
+  Timeout = MMCI0_TIMEOUT;
+  CmdArg = MmcHostInstance->CardInfo.RCA << 16;
+  Response[0] = 0;
+  while(   (!(Response[0] & MMC_R0_READY_FOR_DATA))
+        && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN)
+        && Timeout--) {
+    Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg);
+    if (!EFI_ERROR (Status)) {
+      MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
+      if ((Response[0] & MMC_R0_READY_FOR_DATA)) {
+        break;  // Prevents delay once finished
+      }
+    }
+    gBS->Stall (1);
+  }
+
+  if (BufferSize > This->Media->BlockSize) {
+    Status = MmcHost->SendCommand (MmcHost, MMC_CMD12, 0);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_BLKIO, "%a(): Error and Status:%r\n", __func__, Status));
+    }
+  }
+
+  Status = MmcNotifyState (MmcHostInstance, MmcTransferState);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcTransferState\n"));
+    return Status;
+  }
+  return Status;
+}
+
 EFI_STATUS
 MmcIoBlocks (
   IN EFI_BLOCK_IO_PROTOCOL    *This,
@@ -145,6 +234,7 @@ MmcIoBlocks (
   EFI_MMC_HOST_PROTOCOL   *MmcHost;
   UINTN                   BytesRemainingToBeTransfered;
   UINTN                   BlockCount;
+  UINTN                   ConsumeSize;
 
   BlockCount = 1;
   MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS (This);
@@ -165,6 +255,10 @@ MmcIoBlocks (
     return EFI_NO_MEDIA;
   }
 
+  if (MmcHost->IsMultiBlock && MmcHost->IsMultiBlock(MmcHost)) {
+    BlockCount = (BufferSize + This->Media->BlockSize - 1) / This->Media->BlockSize;
+  }
+
   // All blocks must be within the device
   if ((Lba + (BufferSize / This->Media->BlockSize)) > (This->Media->LastBlock + 1)) {
     return EFI_INVALID_PARAMETER;
@@ -210,75 +304,38 @@ MmcIoBlocks (
       return EFI_NOT_READY;
     }
 
-    //Set command argument based on the card access mode (Byte mode or Block mode)
-    if (MmcHostInstance->CardInfo.OCRData.AccessMode & BIT1) {
-      CmdArg = Lba;
-    } else {
-      CmdArg = Lba * This->Media->BlockSize;
-    }
-
     if (Transfer == MMC_IOBLOCKS_READ) {
-      // Read a single block
-      Cmd = MMC_CMD17;
-    } else {
-      // Write a single block
-      Cmd = MMC_CMD24;
-    }
-    Status = MmcHost->SendCommand (MmcHost, Cmd, CmdArg);
-    if (EFI_ERROR (Status)) {
-      DEBUG ((EFI_D_ERROR, "MmcIoBlocks(MMC_CMD%d): Error %r\n", Cmd, Status));
-      return Status;
-    }
-
-    if (Transfer == MMC_IOBLOCKS_READ) {
-      // Read one block of Data
-      Status = MmcHost->ReadBlockData (MmcHost, Lba, This->Media->BlockSize, Buffer);
-      if (EFI_ERROR (Status)) {
-        DEBUG ((EFI_D_BLKIO, "MmcIoBlocks(): Error Read Block Data and Status = %r\n", Status));
-        MmcStopTransmission (MmcHost);
-        return Status;
-      }
-      Status = MmcNotifyState (MmcHostInstance, MmcProgrammingState);
-      if (EFI_ERROR (Status)) {
-        DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcProgrammingState\n"));
-        return Status;
+      if (BlockCount == 1) {
+        // Read a single block
+        Cmd = MMC_CMD17;
+      } else {
+	// Read multiple blocks
+	Cmd = MMC_CMD18;
       }
     } else {
-      // Write one block of Data
-      Status = MmcHost->WriteBlockData (MmcHost, Lba, This->Media->BlockSize, Buffer);
-      if (EFI_ERROR (Status)) {
-        DEBUG ((EFI_D_BLKIO, "MmcIoBlocks(): Error Write Block Data and Status = %r\n", Status));
-        MmcStopTransmission (MmcHost);
-        return Status;
+      if (BlockCount == 1) {
+        // Write a single block
+        Cmd = MMC_CMD24;
+      } else {
+	// Write multiple blocks
+	Cmd = MMC_CMD25;
       }
     }
 
-    // Command 13 - Read status and wait for programming to complete (return to tran)
-    Timeout = MMCI0_TIMEOUT;
-    CmdArg = MmcHostInstance->CardInfo.RCA << 16;
-    Response[0] = 0;
-    while(   (!(Response[0] & MMC_R0_READY_FOR_DATA))
-          && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN)
-          && Timeout--) {
-      Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg);
-      if (!EFI_ERROR (Status)) {
-        MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
-        if ((Response[0] & MMC_R0_READY_FOR_DATA)) {
-          break;  // Prevents delay once finished
-        }
-      }
-      gBS->Stall (1);
+    ConsumeSize = BlockCount * This->Media->BlockSize;
+    if (BytesRemainingToBeTransfered < ConsumeSize) {
+      ConsumeSize = BytesRemainingToBeTransfered;
     }
-
-    Status = MmcNotifyState (MmcHostInstance, MmcTransferState);
+    Status = MmcTransferBlock (This, Cmd, Transfer, MediaId, Lba, ConsumeSize, Buffer);
     if (EFI_ERROR (Status)) {
-      DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcTransferState\n"));
-      return Status;
+      DEBUG ((EFI_D_ERROR, "%a(): Failed to transfer block and Status:%r\n", __func__, Status));
     }
 
-    BytesRemainingToBeTransfered -= This->Media->BlockSize;
-    Lba    += BlockCount;
-    Buffer = (UINT8 *)Buffer + This->Media->BlockSize;
+    BytesRemainingToBeTransfered -= ConsumeSize;
+    if (BytesRemainingToBeTransfered > 0) {
+      Lba    += BlockCount;
+      Buffer = (UINT8 *)Buffer + ConsumeSize;
+    }
   }
 
   return EFI_SUCCESS;
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 10/11] PL180: update for indentifying SD
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (8 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 09/11] MmcDxe: expand to support multiple blocks Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 15:21 ` [PATCH v4 11/11] PL180: set variable length for CMD6 and ACMD51 Haojian Zhuang
  2016-11-08 17:32 ` [PATCH v4 00/11] enhance MMC Ryan Harkin
  11 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

When CMD6 & ACMD51 are added into indentifying SD process, PL180
should also support CMD6 & ACMD51. Otherwise, it'll hang when
system tries to read expected data.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
index 5526aac..4d839e7 100644
--- a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
+++ b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
@@ -126,6 +126,15 @@ MciSendCommand (
     MciPrepareDataPath (MCI_DATACTL_CARD_TO_CONT);
   } else if ((MmcCmd == MMC_CMD24) || (MmcCmd == MMC_CMD20)) {
     MciPrepareDataPath (MCI_DATACTL_CONT_TO_CARD);
+  } else if (MmcCmd == MMC_CMD6) {
+    MmioWrite32 (MCI_DATA_TIMER_REG, 0xFFFFFFF);
+    MmioWrite32 (MCI_DATA_LENGTH_REG, 64);
+    MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | MCI_DATACTL_STREAM_TRANS);
+  } else if (MmcCmd == MMC_ACMD51) {
+    MmioWrite32 (MCI_DATA_TIMER_REG, 0xFFFFFFF);
+    /* SCR register is 8 bytes long. */
+    MmioWrite32 (MCI_DATA_LENGTH_REG, 8);
+    MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | MCI_DATACTL_STREAM_TRANS);
   }
 
   // Create Command for PL180
@@ -223,7 +232,11 @@ MciReadBlockData (
 
   // Read data from the RX FIFO
   Loop   = 0;
-  Finish = MMCI0_BLOCKLEN / 4;
+  if (Length < MMCI0_BLOCKLEN) {
+    Finish = Length / 4;
+  } else {
+    Finish = MMCI0_BLOCKLEN / 4;
+  }
 
   // Raise the TPL at the highest level to disable Interrupts.
   Tpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 11/11] PL180: set variable length for CMD6 and ACMD51
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (9 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 10/11] PL180: update for indentifying SD Haojian Zhuang
@ 2016-11-08 15:21 ` Haojian Zhuang
  2016-11-08 17:32 ` [PATCH v4 00/11] enhance MMC Ryan Harkin
  11 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-08 15:21 UTC (permalink / raw)
  To: ryan.harkin, edk2-devel, leif.lindholm, ard.biesheuvel; +Cc: Haojian Zhuang

Since CMD6 & ACMD51 needs to read data size less than 512, proper
variable length should be set.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
---
 ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
index 4d839e7..b2ba4c0 100644
--- a/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
+++ b/ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c
@@ -63,11 +63,6 @@ MciIsReadOnly (
   return (MmioRead32 (FixedPcdGet32 (PcdPL180SysMciRegAddress)) & SYS_MCI_WPROT);
 }
 
-#if 0
-//Note: This function has been commented out because it is not used yet.
-//      This function could be used to remove the hardcoded BlockLen used
-//      in MciPrepareDataPath
-
 // Convert block size to 2^n
 STATIC
 UINT32
@@ -87,7 +82,6 @@ GetPow2BlockLen (
 
   return Pow2BlockLen;
 }
-#endif
 
 VOID
 MciPrepareDataPath (
@@ -129,12 +123,20 @@ MciSendCommand (
   } else if (MmcCmd == MMC_CMD6) {
     MmioWrite32 (MCI_DATA_TIMER_REG, 0xFFFFFFF);
     MmioWrite32 (MCI_DATA_LENGTH_REG, 64);
+#ifndef USE_STREAM
+    MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | GetPow2BlockLen (64));
+#else
     MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | MCI_DATACTL_STREAM_TRANS);
+#endif
   } else if (MmcCmd == MMC_ACMD51) {
     MmioWrite32 (MCI_DATA_TIMER_REG, 0xFFFFFFF);
     /* SCR register is 8 bytes long. */
     MmioWrite32 (MCI_DATA_LENGTH_REG, 8);
+#ifndef USE_STREAM
+    MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | GetPow2BlockLen (8));
+#else
     MmioWrite32 (MCI_DATA_CTL_REG, MCI_DATACTL_ENABLE | MCI_DATACTL_CARD_TO_CONT | MCI_DATACTL_STREAM_TRANS);
+#endif
   }
 
   // Create Command for PL180
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 00/11] enhance MMC
  2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
                   ` (10 preceding siblings ...)
  2016-11-08 15:21 ` [PATCH v4 11/11] PL180: set variable length for CMD6 and ACMD51 Haojian Zhuang
@ 2016-11-08 17:32 ` Ryan Harkin
  2016-11-09  0:30   ` Haojian Zhuang
  11 siblings, 1 reply; 22+ messages in thread
From: Ryan Harkin @ 2016-11-08 17:32 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: edk2-devel-01, Leif Lindholm, Ard Biesheuvel

Hi Haojian,

I've tested your v4 series.

On 8 November 2016 at 15:21, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> v4:
>   * Fix PL180 hang in some cases. Since the proper variable length
>     isn't set for CMD6 & ACMD51.
>
> v3:
>   * Fix PL180 hang because of CMD6 & ACMD51 not supported.
>
> v2:
>   * Fix print error with missing parameter.
>   * Change CMD51 to ACMD51.
>   * Add the protection after CMD55 for SD. If there's no
>     response of CMD55, skip to send ACMD51.
>
> v1:
>   * Wait OCR busy bit free according to eMMC spec.
>   * Define ECSD structure.
>   * Add interface to set IO bus width and speed.
>   * Support to access multiple blocks.
>
> Haojian Zhuang (11):
>   MmcDxe: wait OCR busy bit free
>   MmcDxe: move ECSD into CardInfo structure
>   MmcDxe: add SPEC_VERS field in CSD structure
>   MmcDxe: add interface to change io width and speed
>   MmcDxe: declare ECSD structure
>   MmcDxe: set io bus width before reading EXTCSD
>   MmcDxe: Fix uninitialized mediaid for SD
>   MmcDxe: set iospeed and bus width in SD stack

After this patch is applied, TC2 no longer boots.

I can see this patch add support for MMC_ACMD51

>   MmcDxe: expand to support multiple blocks
>   PL180: update for indentifying SD
>   PL180: set variable length for CMD6 and ACMD51

I can see this patch is fixing the data size for ACMD51. And it is
only when this patch is applied that it starts to boot again.

So I think they still need to be stacked or squashed differently so
they are bisect-able, sorry.


>
>  ArmPlatformPkg/Drivers/PL180MciDxe/PL180Mci.c    |  29 +-
>  EmbeddedPkg/Include/Protocol/MmcHost.h           |  29 ++
>  EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 176 ++++++++++-
>  EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c        | 175 +++++++----
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 354 +++++++++++++++++++++--
>  5 files changed, 669 insertions(+), 94 deletions(-)
>
> --
> 2.7.4
>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 01/11] MmcDxe: wait OCR busy bit free
  2016-11-08 15:21 ` [PATCH v4 01/11] MmcDxe: wait OCR busy bit free Haojian Zhuang
@ 2016-11-08 19:27   ` Leif Lindholm
  0 siblings, 0 replies; 22+ messages in thread
From: Leif Lindholm @ 2016-11-08 19:27 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: ryan.harkin, edk2-devel, ard.biesheuvel

On Tue, Nov 08, 2016 at 11:21:06PM +0800, Haojian Zhuang wrote:
> According to eMMC spec, OCR.PowerUp bit is also busy bit. If the busy
> bit is '0', CMD1 should be sent and OCR should be fetched again. And add
> a timeout counter on the repeated steps.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

I will wait for a final Tested-by: from Ryan, but from a code point of
view:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 2d8038f..3f72b7f 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -222,14 +222,19 @@ MmcIdentificationMode (
>  
>    // Send CMD1 to get OCR (MMC)
>    // This command only valid for MMC and eMMC
> -  Status = MmcHost->SendCommand (MmcHost, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
> -  if (Status == EFI_SUCCESS) {
> +  Timeout = MAX_RETRY_COUNT;
> +  do {
> +    Status = MmcHost->SendCommand (MmcHost, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
> +    if (EFI_ERROR (Status))
> +      break;
>      Status = MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_OCR, (UINT32 *)&OcrResponse);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((EFI_D_ERROR, "MmcIdentificationMode() : Failed to receive OCR, Status=%r.\n", Status));
>        return Status;
>      }
> -
> +    Timeout--;
> +  } while (!OcrResponse.Ocr.PowerUp && (Timeout > 0));
> +  if (Status == EFI_SUCCESS) {
>      if (!OcrResponse.Ocr.PowerUp) {
>        DEBUG ((EFI_D_ERROR, "MmcIdentificationMode(MMC_CMD1): Card initialisation failure, Status=%r.\n", Status));
>        return EFI_DEVICE_ERROR;
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 02/11] MmcDxe: move ECSD into CardInfo structure
  2016-11-08 15:21 ` [PATCH v4 02/11] MmcDxe: move ECSD into CardInfo structure Haojian Zhuang
@ 2016-11-08 20:27   ` Leif Lindholm
  0 siblings, 0 replies; 22+ messages in thread
From: Leif Lindholm @ 2016-11-08 20:27 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: ryan.harkin, edk2-devel, ard.biesheuvel

On Tue, Nov 08, 2016 at 11:21:07PM +0800, Haojian Zhuang wrote:
> Since ECSD also describes the information of card, move it into
> structure CardInfo.

I like the fact that you have split out the "move ECSD to CARD_INFO"
and "declare ECSD structure", to keep the two logically separate
modifications apart.

However, it would be better if "declare ECSD structure" followed
directly after this one.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 1 +
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++---
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> index 46a156c..4f132c6 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> @@ -138,6 +138,7 @@ typedef struct  {
>    OCR       OCRData;
>    CID       CIDData;
>    CSD       CSDData;
> +  UINT32    ECSD[128];      // MMC V4 extended card specific
>  } CARD_INFO;
>  
>  typedef struct _MMC_HOST_INSTANCE {
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 3f72b7f..578fcb6 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -39,7 +39,6 @@ EmmcIdentificationMode (
>    EFI_BLOCK_IO_MEDIA    *Media;
>    EFI_STATUS Status;
>    UINT32     RCA;
> -  UINT32     ECSD[128];
>  
>    Host  = MmcHostInstance->MmcHost;
>    Media = MmcHostInstance->BlockIo.Media;
> @@ -91,7 +90,7 @@ EmmcIdentificationMode (
>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
>    }
>  
> -  Status = Host->ReadBlockData (Host, 0, 512, ECSD);
> +  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSD));
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
>      return Status;
> @@ -104,7 +103,7 @@ EmmcIdentificationMode (
>    Media->LogicalBlocksPerPhysicalBlock = 1;
>    Media->IoAlign = 4;
>    // Compute last block using bits [215:212] of the ECSD
> -  Media->LastBlock = ECSD[EMMC_ECSD_SIZE_OFFSET] - 1; // eMMC isn't supposed to report this for
> +  Media->LastBlock = MmcHostInstance->CardInfo.ECSD[EMMC_ECSD_SIZE_OFFSET] - 1; // eMMC isn't supposed to report this for
>    // Cards <2GB in size, but the model does.
>  
>    // Setup card type
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 03/11] MmcDxe: add SPEC_VERS field in CSD structure
  2016-11-08 15:21 ` [PATCH v4 03/11] MmcDxe: add SPEC_VERS field in CSD structure Haojian Zhuang
@ 2016-11-08 20:27   ` Leif Lindholm
  0 siblings, 0 replies; 22+ messages in thread
From: Leif Lindholm @ 2016-11-08 20:27 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: ryan.harkin, edk2-devel, ard.biesheuvel

On Tue, Nov 08, 2016 at 11:21:08PM +0800, Haojian Zhuang wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  EmbeddedPkg/Universal/MmcDxe/Mmc.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> index 4f132c6..8f30a9a 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> @@ -128,7 +128,8 @@ typedef struct {
>    UINT8   NSAC                ;  // Data read access-time 2 in CLK cycles (NSAC*100) [111:104]
>    UINT8   TAAC                ;  // Data read access-time 1 [119:112]
>  
> -  UINT8   RESERVED_5:         6; // Reserved [125:120]
> +  UINT8   RESERVED_5:         2; // Reserved [121:120]
> +  UINT8   SPEC_VERS:          4; // System specification version [125:122]
>    UINT8   CSD_STRUCTURE:      2; // CSD structure [127:126]
>  } CSD;
>  
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 05/11] MmcDxe: declare ECSD structure
  2016-11-08 15:21 ` [PATCH v4 05/11] MmcDxe: declare ECSD structure Haojian Zhuang
@ 2016-11-08 20:43   ` Leif Lindholm
  2016-11-09  0:34     ` Haojian Zhuang
  0 siblings, 1 reply; 22+ messages in thread
From: Leif Lindholm @ 2016-11-08 20:43 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: ryan.harkin, edk2-devel, ard.biesheuvel

On Tue, Nov 08, 2016 at 11:21:10PM +0800, Haojian Zhuang wrote:
> Declare fields in ECSD structure. And drop the original 128 words
> arrary.

Functionality-wise, I'm happy with this, if it is moved to directly
after "add SPEC_VERS field in CSD structure".

A few comments inline with regards to improving readability.

> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 157 ++++++++++++++++++++++-
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c |   4 +-
>  2 files changed, 158 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> index 8f30a9a..d3e9b71 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
> @@ -133,13 +133,168 @@ typedef struct {
>    UINT8   CSD_STRUCTURE:      2; // CSD structure [127:126]
>  } CSD;
>  
> +typedef struct {
> +  UINT8   RESERVED_1[16];                     // Reserved [15:0]
> +  UINT8   SECURE_REMOVAL_TYPE;                // Secure Removal Type [16:16]
> +  UINT8   PRODUCT_STATE_AWARENESS_ENABLEMENT; // Product state awareness enablement [17:17]
> +  UINT8   MAX_PRE_LOADING_DATA_SIZE[4];       // MAX pre loading data size [21:18]
> +  UINT8   PRE_LOADING_DATA_SIZE[4];           // Pre loading data size [25:22]
> +  UINT8   FFU_STATUS;                         // FFU Status [26:26]
> +  UINT8   RESERVED_2[2];                      // Reserved [28:27]
> +  UINT8   MODE_OPERATION_CODES;               // Mode operation codes [29:29]
> +  UINT8   MODE_CONFIG;                        // Mode config [30:30]
> +  UINT8   RESERVED_3;                         // Reserved [31:31]
> +  UINT8   FLUSH_CACHE;                        // Flushing of the cache [32:32]
> +  UINT8   CACHE_CTRL;                         // Control to turn the cache ON/OFF [33:33]
> +  UINT8   POWER_OFF_NOTIFICATION;             // Power Off Notification [34:34]
> +  UINT8   PACKED_FAILURE_INDEX;               // Packed command failure index [35:35]
> +  UINT8   PACKED_COMMAND_STATUS;              // Packed command status [36:36]
> +  UINT8   CONTEXT_CONF[15];                   // Context configuration [51:37]
> +  UINT8   EXT_PARTITIONS_ATTRIBUTE[2];        // Extended partitions attribute [53:52]
> +  UINT8   EXCEPTION_EVENTS_STATUS[2];         // Exception events status [55:54]
> +  UINT8   EXCEPTION_EVENTS_CTRL[2];           // Exception events control [57:56]
> +  UINT8   DYNCAP_NEEDED;                      // Number of addressed group to be released [58:58]
> +  UINT8   CLASS_6_CTRL;                       // Class 6 commands control [59:59]
> +  UINT8   INI_TIMEOUT_EMU;                    // 1st initialization after disabling sector size emulation [60:60]
> +  UINT8   DATA_SECTOR_SIZE;                   // Sector size [61:61]
> +  UINT8   USE_NATIVE_SECTOR;                  // Sector size emulation [62:62]
> +  UINT8   NATIVE_SECTOR_SIZE;                 // Native sector size [63:63]
> +  UINT8   VENDOR_SPECIFIC_FIELD[64];          // Vendor specific fields [127:64]
> +  UINT8   RESERVED_4[2];                      // Reserved [129:128]
> +  UINT8   PROGRAM_CID_CSD_DDR_SUPPORT;        // Program CID/CSD in DDR mode support [130:130]
> +  UINT8   PERIODIC_WAKEUP;                    // Periodic wake-up [131:131]
> +  UINT8   TCASE_SUPPORT;                      // Package case temperature is controlled [132:132]
> +  UINT8   PRODUCTION_STATE_AWARENESS;         // Production state awareness [133:133]
> +  UINT8   SEC_BAD_BLK_MGMNT;                  // Bad block management mode [134:134]
> +  UINT8   RESERVED_5;                         // Reserved [135:135]
> +  UINT8   ENH_START_ADDR[4];                  // Enhanced user data start address [139:136]
> +  UINT8   ENH_SIZE_MULT[3];                   // Enhanced user data area size [142:140]
> +  UINT8   GP_SIZE_MULT[12];                   // General purpose partition size [154:143]
> +  UINT8   PARTITION_SETTING_COMPLETED;        // Partitioning setting [155:155]
> +  UINT8   PARTITIONS_ATTRIBUTE;               // Partitions attribute [156:156]
> +  UINT8   MAX_ENH_SIZE_MULT[3];               // Max enhanced area size [159:157]
> +  UINT8   PARTITIONING_SUPPORT;               // Partitioning [160:160]
> +  UINT8   HPI_MGMT;                           // HPI management [161:161]
> +  UINT8   RST_N_FUNCTION;                     // H/W reset function [162:162]
> +  UINT8   BKOPS_EN;                           // Enable background operations handshake [163:163]
> +  UINT8   BKOPS_START;                        // Manually start background operations [164:164]
> +  UINT8   SANITIZE_START;                     // Start sanitize operation [165:165]
> +  UINT8   WR_REL_PARAM;                       // Write reliability parameter register [166:166]
> +  UINT8   WR_REL_SET;                         // Write reliability setting register [167:167]
> +  UINT8   RPMB_SIZE_MULT;                     // RPMB size [168:168]
> +  UINT8   FW_CONFIG;                          // FW configuration [169:169]
> +  UINT8   RESERVED_6;                         // Reserved [170:170]
> +  UINT8   USER_WP;                            // User area write protection register [171:171]
> +  UINT8   RESERVED_7;                         // Reserved [172:172]
> +  UINT8   BOOT_WP;                            // Boot area write protection register [173:173]
> +  UINT8   BOOT_WP_STATUS;                     // Boot write protection register [174:174]
> +  UINT8   ERASE_GROUP_DEF;                    // High-density erase group definition [175:175]
> +  UINT8   RESERVED_8;                         // Reserved [176:176]
> +  UINT8   BOOT_BUS_CONDITIONS;                // Boot bus conditions [177:177]
> +  UINT8   BOOT_CONFIG_PROT;                   // Boot config protection [178:178]
> +  UINT8   PARTITION_CONFIG;                   // Partition config [179:179]
> +  UINT8   RESERVED_9;                         // Reserved [180:180]
> +  UINT8   ERASED_MEM_CONT;                    // Erased memory content [181:181]
> +  UINT8   RESERVED_10;                        // Reserved [182:182]
> +  UINT8   BUS_WIDTH;                          // Bus width mode [183:183]
> +  UINT8   RESERVED_11;                        // Reserved [184:184]
> +  UINT8   HS_TIMING;                          // High-speed interface timing [185:185]
> +  UINT8   RESERVED_12;                        // Reserved [186:186]
> +  UINT8   POWER_CLASS;                        // Power class [187:187]
> +  UINT8   RESERVED_13;                        // Reserved [188:188]
> +  UINT8   CMD_SET_REV;                        // Command set revision [189:189]
> +  UINT8   RESERVED_14;                        // Reserved [190:190]
> +  UINT8   CMD_SET;                            // Command set [191:191]
> +  UINT8   EXT_CSD_REV;                        // Extended CSD revision [192:192]
> +  UINT8   RESERVED_15;                        // Reserved [193:193]
> +  UINT8   CSD_STRUCTURE;                      // CSD Structure [194:194]
> +  UINT8   RESERVED_16;                        // Reserved [195:195]
> +  UINT8   DEVICE_TYPE;                        // Device type [196:196]
> +  UINT8   DRIVER_STRENGTH;                    // I/O Driver strength [197:197]
> +  UINT8   OUT_OF_INTERRUPT_TIME;              // Out-of-interrupt busy timing [198:198]
> +  UINT8   PARTITION_SWITCH_TIME;              // Partition switching timing [199:199]
> +  UINT8   PWR_CL_52_195;                      // Power class for 52MHz at 1.95V 1 R [200:200]
> +  UINT8   PWR_CL_26_195;                      // Power class for 26MHz at 1.95V 1 R [201:201]
> +  UINT8   PWR_CL_52_360;                      // Power class for 52MHz at 3.6V 1 R [202:202]
> +  UINT8   PWR_CL_26_360;                      // Power class for 26MHz at 3.6V 1 R [203:203]
> +  UINT8   RESERVED_17;                        // Reserved [204:204]
> +  UINT8   MIN_PERF_R_4_26;                    // Minimum read performance for 4bit at 26MHz [205:205]
> +  UINT8   MIN_PERF_W_4_26;                    // Minimum write performance for 4bit at 26MHz [206:206]
> +  UINT8   MIN_PERF_R_8_26_4_52;               // Minimum read performance for 8bit at 26MHz, for 4bit at 52MHz [207:207]
> +  UINT8   MIN_PERF_W_8_26_4_52;               // Minimum write performance for 8bit at 26MHz, for 4bit at 52MHz [208:208]
> +  UINT8   MIN_PERF_R_8_52;                    // Minimum read performance for 8bit at 52MHz [209:209]
> +  UINT8   MIN_PERF_W_8_52;                    // Minimum write performance for 8bit at 52MHz [210:210]
> +  UINT8   RESERVED_18;                        // Reserved [211:211]
> +  UINT32  SEC_COUNT;                          // Sector count [215:212]

So, this field is the only one we currently look at.
In the code below, this replaces the field previously identified vi
EMMC_ECSD_SIZE_OFFSET. Where does this change in name come from?

Could all SEC_ fields referring to SECTOR rbe renamed SECTOR_, and all
referring to SECURE be renamed SECURE_?

> +  UINT8   SLEEP_NOTIFICATION_TIME;            // Sleep notification timout [216:216]
> +  UINT8   S_A_TIMEOUT;                        // Sleep/awake timeout [217:217]
> +  UINT8   PRODUCTION_STATE_AWARENESS_TIMEOUT; // Production state awareness timeout [218:218]
> +  UINT8   S_C_VCCQ;                           // Sleep current (VCCQ) [219:219]
> +  UINT8   S_C_VCC;                            // Sleep current (VCC) [220:220]
> +  UINT8   HC_WP_GRP_SIZE;                     // High-capacity write protect group size [221:221]
> +  UINT8   REL_WR_SEC_C;                       // Reliable write sector count [222:222]
> +  UINT8   ERASE_TIMEOUT_MULT;                 // High-capacity erase timeout [223:223]
> +  UINT8   HC_ERASE_GRP_SIZE;                  // High-capacity erase unit size [224:224]
> +  UINT8   ACC_SIZE;                           // Access size [225:225]
> +  UINT8   BOOT_SIZE_MULTI;                    // Boot partition size [226:226]
> +  UINT8   RESERVED_19;                        // Reserved [227:227]
> +  UINT8   BOOT_INFO;                          // Boot information [228:228]
> +  UINT8   SEC_TRIM_MULT;                      // Secure TRIM Multiplier [229:229]
> +  UINT8   SEC_ERASE_MULT;                     // Secure Erase Multiplier [230:230]
> +  UINT8   SEC_FEATURE_SUPPORT;                // Secure Feature Support [231:231]
> +  UINT8   TRIM_MULT;                          // TRIM Multiplier [232:232]
> +  UINT8   RESERVED_20;                        // Reserved [233:233]
> +  UINT8   MIN_PREF_DDR_R_8_52;                // Minimum read performance for 8bit at 52MHz in DDR mode [234:234]
> +  UINT8   MIN_PREF_DDR_W_8_52;                // Minimum write performance for 8bit at 52MHz in DDR mode [235:235]
> +  UINT8   PWR_CL_200_130;                     // Power class for 200MHz at VCCQ=1.3V, VCC=3.6V [236:236]
> +  UINT8   PWR_CL_200_195;                     // Power class for 200MHz at VCCQ=1.95V, VCC=3.6V [237:237]
> +  UINT8   PWR_CL_DDR_52_195;                  // Power class for 52MHz, DDR at 1.95V [238:238]
> +  UINT8   PWR_CL_DDR_52_360;                  // Power class for 52Mhz, DDR at 3.6V [239:239]
> +  UINT8   RESERVED_21;                        // Reserved [240:240]
> +  UINT8   INI_TIMEOUT_AP;                     // 1st initialization time after partitioning [241:241]
> +  UINT8   CORRECTLY_PRG_SECTORS_NUM[4];       // Number of correctly programmed sectors [245:242]
> +  UINT8   BKOPS_STATUS;                       // Background operations status [246:246]
> +  UINT8   POWER_OFF_LONG_TIME;                // Power off notification (long) timeout [247:247]
> +  UINT8   GENERIC_CMD6_TIME;                  // Generic CMD6 timeout [248:248]
> +  UINT8   CACHE_SIZE[4];                      // Cache size [252:249]
> +  UINT8   PWR_CL_DDR_200_360;                 // Power class for 200MHz, DDR at VCC=3.6V [253:253]
> +  UINT8   FIRMWARE_VERSION[8];                // Firmware version [261:254]
> +  UINT8   DEVICE_VERSION[2];                  // Device version [263:262]
> +  UINT8   OPTIMAL_TRIM_UNIT_SIZE;             // Optimal trim unit size [264:264]
> +  UINT8   OPTIMAL_WRITE_SIZE;                 // Optimal write size [265:265]
> +  UINT8   OPTIMAL_READ_SIZE;                  // Optimal read size [266:266]
> +  UINT8   PRE_EOL_INFO;                       // Pre EOL information [267:267]
> +  UINT8   DEVICE_LIFE_TIME_EST_TYP_A;         // Device life time estimation type A [268:268]
> +  UINT8   DEVICE_LIFE_TIME_EST_TYP_B;         // Device life time estimation type B [269:269]
> +  UINT8   VENDOR_PROPRIETARY_HEALTH_REPORT[32];         // Vendor proprietary health report [301:270]
> +  UINT8   NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED[4]; // Number of FW sectors correctly programmed [305:302]
> +  UINT8   RESERVED_22[181];                   // Reserved [486:306]
> +  UINT8   FFU_ARG[4];                         // FFU argument [490:487]
> +  UINT8   OPERATION_CODE_TIMEOUT;             // Operation codes timeout [491:491]
> +  UINT8   FFU_FEATURES;                       // FFU features [492:492]
> +  UINT8   SUPPORTED_MODES;                    // Supported modes [493:493]
> +  UINT8   EXT_SUPPORT;                        // Extended partitions attribute support [494:494]
> +  UINT8   LARGE_UNIT_SIZE_M1;                 // Large unit size [495:495]
> +  UINT8   CONTEXT_CAPABILITIES;               // Context management capabilities [496:496]
> +  UINT8   TAG_RES_SIZE;                       // Tag resource size [497:497]
> +  UINT8   TAG_UNIT_SIZE;                      // Tag unit size [498:498]
> +  UINT8   DATA_TAG_SUPPORT;                   // Data tag support [499:499]
> +  UINT8   MAX_PACKED_WRITES;                  // Max packed write commands [500:500]
> +  UINT8   MAX_PACKED_READS;                   // Max packed read commands [501:501]
> +  UINT8   BKOPS_SUPPORT;                      // Background operations support [502:502]
> +  UINT8   HPI_FEATURES;                       // HPI features [503:503]
> +  UINT8   S_CMD_SET;                          // Supported command sets [504:504]
> +  UINT8   EXT_SECURITY_ERR;                   // Extended security commands error [505:505]
> +  UINT8   RESERVED_23[6];                     // Reserved [511:506]
> +} ECSD;
> +
>  typedef struct  {
>    UINT16    RCA;
>    CARD_TYPE CardType;
>    OCR       OCRData;
>    CID       CIDData;
>    CSD       CSDData;
> -  UINT32    ECSD[128];      // MMC V4 extended card specific
> +  ECSD      ECSDData;                         // MMC V4 extended card specific
>  } CARD_INFO;
>  
>  typedef struct _MMC_HOST_INSTANCE {
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 578fcb6..c5897de 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -90,7 +90,7 @@ EmmcIdentificationMode (
>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
>    }
>  
> -  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSD));
> +  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
>      return Status;
> @@ -103,7 +103,7 @@ EmmcIdentificationMode (
>    Media->LogicalBlocksPerPhysicalBlock = 1;
>    Media->IoAlign = 4;
>    // Compute last block using bits [215:212] of the ECSD
> -  Media->LastBlock = MmcHostInstance->CardInfo.ECSD[EMMC_ECSD_SIZE_OFFSET] - 1; // eMMC isn't supposed to report this for
> +  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SEC_COUNT - 1; // eMMC isn't supposed to report this for

This deletes the only use of the EMMC_ECSD_SIZE_OFFSET define - could
we delete the definition of that too?

>    // Cards <2GB in size, but the model does.
>  
>    // Setup card type
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD
  2016-11-08 15:21 ` [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD Haojian Zhuang
@ 2016-11-08 23:59   ` Leif Lindholm
  2016-11-13  6:46     ` Haojian Zhuang
  0 siblings, 1 reply; 22+ messages in thread
From: Leif Lindholm @ 2016-11-08 23:59 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: ryan.harkin, edk2-devel, ard.biesheuvel

On Tue, Nov 08, 2016 at 11:21:12PM +0800, Haojian Zhuang wrote:
> When SD card is used, mediaid is not initialized and used directly. So
> fix it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index cefc2b6..5b802c0 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -57,6 +57,7 @@ typedef enum _EMMC_DEVICE_STATE {
>  } EMMC_DEVICE_STATE;
>  
>  UINT32 mEmmcRcaCount = 0;
> +UINT32 CurrentMediaId = 0;

Should have an 'm' prefix.

>  
>  STATIC
>  EFI_STATUS
> @@ -231,6 +232,10 @@ EmmcIdentificationMode (
>    // Set up media
>    Media->BlockSize = EMMC_CARD_SIZE; // 512-byte support is mandatory for eMMC cards
>    Media->MediaId = MmcHostInstance->CardInfo.CIDData.PSN;

I spent some time digging through this code in order to understand
what is going on below. I think this could do with a comment
explaining the logic. Also, always use {} with if/else.

> +  if (CurrentMediaId > Media->MediaId)
> +    Media->MediaId = ++CurrentMediaId;
> +  else
> +    CurrentMediaId = Media->MediaId;

I will give my interpretation of how this works, and you can confirm
or deny it.

When this Media entity is created, it is based on mMmcMediaTemplate.
mMmcMediaTemplate (ab)uses the MediaId field as a Signature - "mmco",
meaning any new MMC device starts with a MediaId of 0x6d6d626f (or
0x6f626d6d if I got the endianness wrong).

So the value is initialised, just to the same for every MMC media in
the system.

Since the module-global (m)CurrentMediaId is initialised to 0, the
first time MmcIdentificationMode() is called for the first eMMC
device, (m)CurrentMediaId will be set to 0x6d6d6270 (or 0x6f626d6e).

Does this not leave it uninitialised for MMC and other media types?

I guess it's not a huge deal if the MediaID clashes for different
devices, because it is mainly meant to indicate a removable media has
changed. But that also means there should not have been a problem in
the first place.

So can you describe in the commit message what failure mode this patch
gets rid of?

Regardless, I think the code would be more clear in what it is doing
if it did:
  if (Media->MediaId == SIGNATURE_32('m','m','c','o')) {
    Media->MediaId = ++CurrentMediaId;
  } else {
    CurrentMediaId = Media->MediaId;
  }

>    Media->ReadOnly = MmcHostInstance->CardInfo.CSDData.PERM_WRITE_PROTECT;
>    Media->LogicalBlocksPerPhysicalBlock = 1;
>    Media->IoAlign = 4;
> @@ -344,7 +349,7 @@ InitializeSdMmcDevice (
>    MmcHostInstance->BlockIo.Media->BlockSize    = BlockSize;
>    MmcHostInstance->BlockIo.Media->ReadOnly     = MmcHost->IsReadOnly (MmcHost);
>    MmcHostInstance->BlockIo.Media->MediaPresent = TRUE;
> -  MmcHostInstance->BlockIo.Media->MediaId++;
> +  MmcHostInstance->BlockIo.Media->MediaId      = ++CurrentMediaId;
>  
>    CmdArg = MmcHostInstance->CardInfo.RCA << 16;
>    Status = MmcHost->SendCommand (MmcHost, MMC_CMD7, CmdArg);
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 09/11] MmcDxe: expand to support multiple blocks
  2016-11-08 15:21 ` [PATCH v4 09/11] MmcDxe: expand to support multiple blocks Haojian Zhuang
@ 2016-11-09  0:14   ` Leif Lindholm
  0 siblings, 0 replies; 22+ messages in thread
From: Leif Lindholm @ 2016-11-09  0:14 UTC (permalink / raw)
  To: Haojian Zhuang; +Cc: ryan.harkin, edk2-devel, ard.biesheuvel

On Tue, Nov 08, 2016 at 11:21:14PM +0800, Haojian Zhuang wrote:
> Make use of DMA to transfer multiple blocks at one time. It could
> improve the performance on MMC/SD driver.

A few style comments inline.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  EmbeddedPkg/Include/Protocol/MmcHost.h    |   6 +
>  EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 175 ++++++++++++++++++++----------
>  2 files changed, 122 insertions(+), 59 deletions(-)
> 
> diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
> index 5028045..cd5335a 100644
> --- a/EmbeddedPkg/Include/Protocol/MmcHost.h
> +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
> @@ -62,6 +62,7 @@ typedef UINT32 MMC_CMD;
>  #define MMC_CMD20             (MMC_INDX(20) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD23             (MMC_INDX(23) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD24             (MMC_INDX(24) | MMC_CMD_WAIT_RESPONSE)
> +#define MMC_CMD25             (MMC_INDX(25) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD55             (MMC_INDX(55) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_ACMD41            (MMC_INDX(41) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE)
>  #define MMC_ACMD51            (MMC_INDX(51) | MMC_CMD_WAIT_RESPONSE)
> @@ -152,6 +153,10 @@ typedef EFI_STATUS (EFIAPI *MMC_SETIOS) (
>    IN  UINT32                    TimingMode
>    );
>  
> +typedef BOOLEAN (EFIAPI *MMC_ISMULTIBLOCK) (
> +  IN  EFI_MMC_HOST_PROTOCOL     *This
> +  );
> +
>  
>  struct _EFI_MMC_HOST_PROTOCOL {
>  
> @@ -169,6 +174,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
>    MMC_WRITEBLOCKDATA      WriteBlockData;
>  
>    MMC_SETIOS              SetIos;
> +  MMC_ISMULTIBLOCK        IsMultiBlock;
>  
>  };
>  
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> index 0e1ef57..043ca92 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> @@ -126,6 +126,95 @@ MmcStopTransmission (
>  #define MMCI0_BLOCKLEN 512
>  #define MMCI0_TIMEOUT  10000
>  
> +STATIC EFI_STATUS
> +MmcTransferBlock (
> +  IN EFI_BLOCK_IO_PROTOCOL    *This,
> +  IN UINTN                    Cmd,
> +  IN UINTN                    Transfer,
> +  IN UINT32                   MediaId,
> +  IN EFI_LBA                  Lba,
> +  IN UINTN                    BufferSize,
> +  OUT VOID                    *Buffer
> +  )
> +{
> +  EFI_STATUS              Status;
> +  UINTN                   CmdArg;
> +  INTN                    Timeout;
> +  UINT32                  Response[4];
> +  MMC_HOST_INSTANCE       *MmcHostInstance;
> +  EFI_MMC_HOST_PROTOCOL   *MmcHost;
> +
> +  MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS (This);
> +  MmcHost = MmcHostInstance->MmcHost;
> +
> +  //Set command argument based on the card access mode (Byte mode or Block mode)
> +  if (MmcHostInstance->CardInfo.OCRData.AccessMode & BIT1) {
> +    CmdArg = Lba;
> +  } else {
> +    CmdArg = Lba * This->Media->BlockSize;
> +  }
> +
> +  Status = MmcHost->SendCommand (MmcHost, Cmd, CmdArg);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "%a(MMC_CMD%d): Error %r\n", __func__, Cmd, Status));
> +    return Status;
> +  }
> +
> +  if (Transfer == MMC_IOBLOCKS_READ) {
> +    // Read Data
> +    Status = MmcHost->ReadBlockData (MmcHost, Lba, BufferSize, Buffer);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_BLKIO, "%a(): Error Read Block Data and Status = %r\n", __func__, Status));
> +      MmcStopTransmission (MmcHost);
> +      return Status;
> +    }
> +    Status = MmcNotifyState (MmcHostInstance, MmcProgrammingState);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "%a() : Error MmcProgrammingState\n", __func__));
> +      return Status;
> +    }
> +  } else {
> +    // Write Data
> +    Status = MmcHost->WriteBlockData (MmcHost, Lba, BufferSize, Buffer);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_BLKIO, "%a(): Error Write Block Data and Status = %r\n", __func__, Status));
> +      MmcStopTransmission (MmcHost);
> +      return Status;
> +    }
> +  }
> +
> +  // Command 13 - Read status and wait for programming to complete (return to tran)
> +  Timeout = MMCI0_TIMEOUT;
> +  CmdArg = MmcHostInstance->CardInfo.RCA << 16;
> +  Response[0] = 0;
> +  while(   (!(Response[0] & MMC_R0_READY_FOR_DATA))

A few too many spaces above, and redundant external parentheses.
     while(!(Response[0] & MMC_R0_READY_FOR_DATA)

> +        && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN)
> +        && Timeout--) {
> +    Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg);
> +    if (!EFI_ERROR (Status)) {
> +      MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
> +      if ((Response[0] & MMC_R0_READY_FOR_DATA)) {

         if (Response[0] & MMC_R0_READY_FOR_DATA) {

> +        break;  // Prevents delay once finished
> +      }
> +    }
> +    gBS->Stall (1);
> +  }
> +
> +  if (BufferSize > This->Media->BlockSize) {
> +    Status = MmcHost->SendCommand (MmcHost, MMC_CMD12, 0);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_BLKIO, "%a(): Error and Status:%r\n", __func__, Status));
> +    }
> +  }
> +
> +  Status = MmcNotifyState (MmcHostInstance, MmcTransferState);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcTransferState\n"));
> +    return Status;
> +  }
> +  return Status;
> +}
> +
>  EFI_STATUS
>  MmcIoBlocks (
>    IN EFI_BLOCK_IO_PROTOCOL    *This,
> @@ -145,6 +234,7 @@ MmcIoBlocks (
>    EFI_MMC_HOST_PROTOCOL   *MmcHost;
>    UINTN                   BytesRemainingToBeTransfered;
>    UINTN                   BlockCount;
> +  UINTN                   ConsumeSize;
>  
>    BlockCount = 1;
>    MmcHostInstance = MMC_HOST_INSTANCE_FROM_BLOCK_IO_THIS (This);
> @@ -165,6 +255,10 @@ MmcIoBlocks (
>      return EFI_NO_MEDIA;
>    }
>  
> +  if (MmcHost->IsMultiBlock && MmcHost->IsMultiBlock(MmcHost)) {
> +    BlockCount = (BufferSize + This->Media->BlockSize - 1) / This->Media->BlockSize;
> +  }
> +
>    // All blocks must be within the device
>    if ((Lba + (BufferSize / This->Media->BlockSize)) > (This->Media->LastBlock + 1)) {
>      return EFI_INVALID_PARAMETER;
> @@ -210,75 +304,38 @@ MmcIoBlocks (
>        return EFI_NOT_READY;
>      }
>  
> -    //Set command argument based on the card access mode (Byte mode or Block mode)
> -    if (MmcHostInstance->CardInfo.OCRData.AccessMode & BIT1) {
> -      CmdArg = Lba;
> -    } else {
> -      CmdArg = Lba * This->Media->BlockSize;
> -    }
> -
>      if (Transfer == MMC_IOBLOCKS_READ) {
> -      // Read a single block
> -      Cmd = MMC_CMD17;
> -    } else {
> -      // Write a single block
> -      Cmd = MMC_CMD24;
> -    }
> -    Status = MmcHost->SendCommand (MmcHost, Cmd, CmdArg);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "MmcIoBlocks(MMC_CMD%d): Error %r\n", Cmd, Status));
> -      return Status;
> -    }
> -
> -    if (Transfer == MMC_IOBLOCKS_READ) {
> -      // Read one block of Data
> -      Status = MmcHost->ReadBlockData (MmcHost, Lba, This->Media->BlockSize, Buffer);
> -      if (EFI_ERROR (Status)) {
> -        DEBUG ((EFI_D_BLKIO, "MmcIoBlocks(): Error Read Block Data and Status = %r\n", Status));
> -        MmcStopTransmission (MmcHost);
> -        return Status;
> -      }
> -      Status = MmcNotifyState (MmcHostInstance, MmcProgrammingState);
> -      if (EFI_ERROR (Status)) {
> -        DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcProgrammingState\n"));
> -        return Status;
> +      if (BlockCount == 1) {
> +        // Read a single block
> +        Cmd = MMC_CMD17;
> +      } else {
> +	// Read multiple blocks
> +	Cmd = MMC_CMD18;

Faulty indentation (tabs). Please re-run PatchCheck.py.

>        }
>      } else {
> -      // Write one block of Data
> -      Status = MmcHost->WriteBlockData (MmcHost, Lba, This->Media->BlockSize, Buffer);
> -      if (EFI_ERROR (Status)) {
> -        DEBUG ((EFI_D_BLKIO, "MmcIoBlocks(): Error Write Block Data and Status = %r\n", Status));
> -        MmcStopTransmission (MmcHost);
> -        return Status;
> +      if (BlockCount == 1) {
> +        // Write a single block
> +        Cmd = MMC_CMD24;
> +      } else {
> +	// Write multiple blocks
> +	Cmd = MMC_CMD25;
>        }
>      }
>  
> -    // Command 13 - Read status and wait for programming to complete (return to tran)
> -    Timeout = MMCI0_TIMEOUT;
> -    CmdArg = MmcHostInstance->CardInfo.RCA << 16;
> -    Response[0] = 0;
> -    while(   (!(Response[0] & MMC_R0_READY_FOR_DATA))
> -          && (MMC_R0_CURRENTSTATE (Response) != MMC_R0_STATE_TRAN)
> -          && Timeout--) {
> -      Status = MmcHost->SendCommand (MmcHost, MMC_CMD13, CmdArg);
> -      if (!EFI_ERROR (Status)) {
> -        MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
> -        if ((Response[0] & MMC_R0_READY_FOR_DATA)) {
> -          break;  // Prevents delay once finished
> -        }
> -      }
> -      gBS->Stall (1);
> +    ConsumeSize = BlockCount * This->Media->BlockSize;
> +    if (BytesRemainingToBeTransfered < ConsumeSize) {
> +      ConsumeSize = BytesRemainingToBeTransfered;
>      }
> -
> -    Status = MmcNotifyState (MmcHostInstance, MmcTransferState);
> +    Status = MmcTransferBlock (This, Cmd, Transfer, MediaId, Lba, ConsumeSize, Buffer);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "MmcIoBlocks() : Error MmcTransferState\n"));
> -      return Status;
> +      DEBUG ((EFI_D_ERROR, "%a(): Failed to transfer block and Status:%r\n", __func__, Status));
>      }
>  
> -    BytesRemainingToBeTransfered -= This->Media->BlockSize;
> -    Lba    += BlockCount;
> -    Buffer = (UINT8 *)Buffer + This->Media->BlockSize;
> +    BytesRemainingToBeTransfered -= ConsumeSize;
> +    if (BytesRemainingToBeTransfered > 0) {
> +      Lba    += BlockCount;
> +      Buffer = (UINT8 *)Buffer + ConsumeSize;
> +    }
>    }
>  
>    return EFI_SUCCESS;
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 00/11] enhance MMC
  2016-11-08 17:32 ` [PATCH v4 00/11] enhance MMC Ryan Harkin
@ 2016-11-09  0:30   ` Haojian Zhuang
  0 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-09  0:30 UTC (permalink / raw)
  To: Ryan Harkin; +Cc: edk2-devel-01, Leif Lindholm, Ard Biesheuvel

On 9 November 2016 at 01:32, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> Hi Haojian,
>
> I've tested your v4 series.
>
> On 8 November 2016 at 15:21, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
>> v4:
>>   * Fix PL180 hang in some cases. Since the proper variable length
>>     isn't set for CMD6 & ACMD51.
>>
>> v3:
>>   * Fix PL180 hang because of CMD6 & ACMD51 not supported.
>>
>> v2:
>>   * Fix print error with missing parameter.
>>   * Change CMD51 to ACMD51.
>>   * Add the protection after CMD55 for SD. If there's no
>>     response of CMD55, skip to send ACMD51.
>>
>> v1:
>>   * Wait OCR busy bit free according to eMMC spec.
>>   * Define ECSD structure.
>>   * Add interface to set IO bus width and speed.
>>   * Support to access multiple blocks.
>>
>> Haojian Zhuang (11):
>>   MmcDxe: wait OCR busy bit free
>>   MmcDxe: move ECSD into CardInfo structure
>>   MmcDxe: add SPEC_VERS field in CSD structure
>>   MmcDxe: add interface to change io width and speed
>>   MmcDxe: declare ECSD structure
>>   MmcDxe: set io bus width before reading EXTCSD
>>   MmcDxe: Fix uninitialized mediaid for SD
>>   MmcDxe: set iospeed and bus width in SD stack
>
> After this patch is applied, TC2 no longer boots.
>
> I can see this patch add support for MMC_ACMD51
>
>>   MmcDxe: expand to support multiple blocks
>>   PL180: update for indentifying SD
>>   PL180: set variable length for CMD6 and ACMD51
>
> I can see this patch is fixing the data size for ACMD51. And it is
> only when this patch is applied that it starts to boot again.
>
> So I think they still need to be stacked or squashed differently so
> they are bisect-able, sorry.
>
OK. I'll pack them into one single patch.

Best Regards
Haojian


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 05/11] MmcDxe: declare ECSD structure
  2016-11-08 20:43   ` Leif Lindholm
@ 2016-11-09  0:34     ` Haojian Zhuang
  0 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-09  0:34 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: Ryan Harkin, edk2-devel-01, Ard Biesheuvel

On 9 November 2016 at 04:43, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Nov 08, 2016 at 11:21:10PM +0800, Haojian Zhuang wrote:
>> Declare fields in ECSD structure. And drop the original 128 words
>> arrary.
>
> Functionality-wise, I'm happy with this, if it is moved to directly
> after "add SPEC_VERS field in CSD structure".
>
> A few comments inline with regards to improving readability.
>
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  EmbeddedPkg/Universal/MmcDxe/Mmc.h               | 157 ++++++++++++++++++++++-
>>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c |   4 +-
>>  2 files changed, 158 insertions(+), 3 deletions(-)
>>
>> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
>> index 8f30a9a..d3e9b71 100644
>> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
>> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
>> @@ -133,13 +133,168 @@ typedef struct {
>>    UINT8   CSD_STRUCTURE:      2; // CSD structure [127:126]
>>  } CSD;
>>
>> +typedef struct {
>> +  UINT8   RESERVED_1[16];                     // Reserved [15:0]
>> +  UINT8   SECURE_REMOVAL_TYPE;                // Secure Removal Type [16:16]
>> +  UINT8   PRODUCT_STATE_AWARENESS_ENABLEMENT; // Product state awareness enablement [17:17]
>> +  UINT8   MAX_PRE_LOADING_DATA_SIZE[4];       // MAX pre loading data size [21:18]
>> +  UINT8   PRE_LOADING_DATA_SIZE[4];           // Pre loading data size [25:22]
>> +  UINT8   FFU_STATUS;                         // FFU Status [26:26]
>> +  UINT8   RESERVED_2[2];                      // Reserved [28:27]
>> +  UINT8   MODE_OPERATION_CODES;               // Mode operation codes [29:29]
>> +  UINT8   MODE_CONFIG;                        // Mode config [30:30]
>> +  UINT8   RESERVED_3;                         // Reserved [31:31]
>> +  UINT8   FLUSH_CACHE;                        // Flushing of the cache [32:32]
>> +  UINT8   CACHE_CTRL;                         // Control to turn the cache ON/OFF [33:33]
>> +  UINT8   POWER_OFF_NOTIFICATION;             // Power Off Notification [34:34]
>> +  UINT8   PACKED_FAILURE_INDEX;               // Packed command failure index [35:35]
>> +  UINT8   PACKED_COMMAND_STATUS;              // Packed command status [36:36]
>> +  UINT8   CONTEXT_CONF[15];                   // Context configuration [51:37]
>> +  UINT8   EXT_PARTITIONS_ATTRIBUTE[2];        // Extended partitions attribute [53:52]
>> +  UINT8   EXCEPTION_EVENTS_STATUS[2];         // Exception events status [55:54]
>> +  UINT8   EXCEPTION_EVENTS_CTRL[2];           // Exception events control [57:56]
>> +  UINT8   DYNCAP_NEEDED;                      // Number of addressed group to be released [58:58]
>> +  UINT8   CLASS_6_CTRL;                       // Class 6 commands control [59:59]
>> +  UINT8   INI_TIMEOUT_EMU;                    // 1st initialization after disabling sector size emulation [60:60]
>> +  UINT8   DATA_SECTOR_SIZE;                   // Sector size [61:61]
>> +  UINT8   USE_NATIVE_SECTOR;                  // Sector size emulation [62:62]
>> +  UINT8   NATIVE_SECTOR_SIZE;                 // Native sector size [63:63]
>> +  UINT8   VENDOR_SPECIFIC_FIELD[64];          // Vendor specific fields [127:64]
>> +  UINT8   RESERVED_4[2];                      // Reserved [129:128]
>> +  UINT8   PROGRAM_CID_CSD_DDR_SUPPORT;        // Program CID/CSD in DDR mode support [130:130]
>> +  UINT8   PERIODIC_WAKEUP;                    // Periodic wake-up [131:131]
>> +  UINT8   TCASE_SUPPORT;                      // Package case temperature is controlled [132:132]
>> +  UINT8   PRODUCTION_STATE_AWARENESS;         // Production state awareness [133:133]
>> +  UINT8   SEC_BAD_BLK_MGMNT;                  // Bad block management mode [134:134]
>> +  UINT8   RESERVED_5;                         // Reserved [135:135]
>> +  UINT8   ENH_START_ADDR[4];                  // Enhanced user data start address [139:136]
>> +  UINT8   ENH_SIZE_MULT[3];                   // Enhanced user data area size [142:140]
>> +  UINT8   GP_SIZE_MULT[12];                   // General purpose partition size [154:143]
>> +  UINT8   PARTITION_SETTING_COMPLETED;        // Partitioning setting [155:155]
>> +  UINT8   PARTITIONS_ATTRIBUTE;               // Partitions attribute [156:156]
>> +  UINT8   MAX_ENH_SIZE_MULT[3];               // Max enhanced area size [159:157]
>> +  UINT8   PARTITIONING_SUPPORT;               // Partitioning [160:160]
>> +  UINT8   HPI_MGMT;                           // HPI management [161:161]
>> +  UINT8   RST_N_FUNCTION;                     // H/W reset function [162:162]
>> +  UINT8   BKOPS_EN;                           // Enable background operations handshake [163:163]
>> +  UINT8   BKOPS_START;                        // Manually start background operations [164:164]
>> +  UINT8   SANITIZE_START;                     // Start sanitize operation [165:165]
>> +  UINT8   WR_REL_PARAM;                       // Write reliability parameter register [166:166]
>> +  UINT8   WR_REL_SET;                         // Write reliability setting register [167:167]
>> +  UINT8   RPMB_SIZE_MULT;                     // RPMB size [168:168]
>> +  UINT8   FW_CONFIG;                          // FW configuration [169:169]
>> +  UINT8   RESERVED_6;                         // Reserved [170:170]
>> +  UINT8   USER_WP;                            // User area write protection register [171:171]
>> +  UINT8   RESERVED_7;                         // Reserved [172:172]
>> +  UINT8   BOOT_WP;                            // Boot area write protection register [173:173]
>> +  UINT8   BOOT_WP_STATUS;                     // Boot write protection register [174:174]
>> +  UINT8   ERASE_GROUP_DEF;                    // High-density erase group definition [175:175]
>> +  UINT8   RESERVED_8;                         // Reserved [176:176]
>> +  UINT8   BOOT_BUS_CONDITIONS;                // Boot bus conditions [177:177]
>> +  UINT8   BOOT_CONFIG_PROT;                   // Boot config protection [178:178]
>> +  UINT8   PARTITION_CONFIG;                   // Partition config [179:179]
>> +  UINT8   RESERVED_9;                         // Reserved [180:180]
>> +  UINT8   ERASED_MEM_CONT;                    // Erased memory content [181:181]
>> +  UINT8   RESERVED_10;                        // Reserved [182:182]
>> +  UINT8   BUS_WIDTH;                          // Bus width mode [183:183]
>> +  UINT8   RESERVED_11;                        // Reserved [184:184]
>> +  UINT8   HS_TIMING;                          // High-speed interface timing [185:185]
>> +  UINT8   RESERVED_12;                        // Reserved [186:186]
>> +  UINT8   POWER_CLASS;                        // Power class [187:187]
>> +  UINT8   RESERVED_13;                        // Reserved [188:188]
>> +  UINT8   CMD_SET_REV;                        // Command set revision [189:189]
>> +  UINT8   RESERVED_14;                        // Reserved [190:190]
>> +  UINT8   CMD_SET;                            // Command set [191:191]
>> +  UINT8   EXT_CSD_REV;                        // Extended CSD revision [192:192]
>> +  UINT8   RESERVED_15;                        // Reserved [193:193]
>> +  UINT8   CSD_STRUCTURE;                      // CSD Structure [194:194]
>> +  UINT8   RESERVED_16;                        // Reserved [195:195]
>> +  UINT8   DEVICE_TYPE;                        // Device type [196:196]
>> +  UINT8   DRIVER_STRENGTH;                    // I/O Driver strength [197:197]
>> +  UINT8   OUT_OF_INTERRUPT_TIME;              // Out-of-interrupt busy timing [198:198]
>> +  UINT8   PARTITION_SWITCH_TIME;              // Partition switching timing [199:199]
>> +  UINT8   PWR_CL_52_195;                      // Power class for 52MHz at 1.95V 1 R [200:200]
>> +  UINT8   PWR_CL_26_195;                      // Power class for 26MHz at 1.95V 1 R [201:201]
>> +  UINT8   PWR_CL_52_360;                      // Power class for 52MHz at 3.6V 1 R [202:202]
>> +  UINT8   PWR_CL_26_360;                      // Power class for 26MHz at 3.6V 1 R [203:203]
>> +  UINT8   RESERVED_17;                        // Reserved [204:204]
>> +  UINT8   MIN_PERF_R_4_26;                    // Minimum read performance for 4bit at 26MHz [205:205]
>> +  UINT8   MIN_PERF_W_4_26;                    // Minimum write performance for 4bit at 26MHz [206:206]
>> +  UINT8   MIN_PERF_R_8_26_4_52;               // Minimum read performance for 8bit at 26MHz, for 4bit at 52MHz [207:207]
>> +  UINT8   MIN_PERF_W_8_26_4_52;               // Minimum write performance for 8bit at 26MHz, for 4bit at 52MHz [208:208]
>> +  UINT8   MIN_PERF_R_8_52;                    // Minimum read performance for 8bit at 52MHz [209:209]
>> +  UINT8   MIN_PERF_W_8_52;                    // Minimum write performance for 8bit at 52MHz [210:210]
>> +  UINT8   RESERVED_18;                        // Reserved [211:211]
>> +  UINT32  SEC_COUNT;                          // Sector count [215:212]
>
> So, this field is the only one we currently look at.
> In the code below, this replaces the field previously identified vi
> EMMC_ECSD_SIZE_OFFSET. Where does this change in name come from?

I found them in JEDEC eMMC spec (JESD84-A44).

>
> Could all SEC_ fields referring to SECTOR rbe renamed SECTOR_, and all
> referring to SECURE be renamed SECURE_?

OK. I'll do the rename.

Best Regards
Haojian


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD
  2016-11-08 23:59   ` Leif Lindholm
@ 2016-11-13  6:46     ` Haojian Zhuang
  0 siblings, 0 replies; 22+ messages in thread
From: Haojian Zhuang @ 2016-11-13  6:46 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: Ryan Harkin, edk2-devel-01, Ard Biesheuvel

On 9 November 2016 at 07:59, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Nov 08, 2016 at 11:21:12PM +0800, Haojian Zhuang wrote:
>> When SD card is used, mediaid is not initialized and used directly. So
>> fix it.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> index cefc2b6..5b802c0 100644
>> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
>> @@ -57,6 +57,7 @@ typedef enum _EMMC_DEVICE_STATE {
>>  } EMMC_DEVICE_STATE;
>>
>>  UINT32 mEmmcRcaCount = 0;
>> +UINT32 CurrentMediaId = 0;
>
> Should have an 'm' prefix.
>
>>
>>  STATIC
>>  EFI_STATUS
>> @@ -231,6 +232,10 @@ EmmcIdentificationMode (
>>    // Set up media
>>    Media->BlockSize = EMMC_CARD_SIZE; // 512-byte support is mandatory for eMMC cards
>>    Media->MediaId = MmcHostInstance->CardInfo.CIDData.PSN;
>
> I spent some time digging through this code in order to understand
> what is going on below. I think this could do with a comment
> explaining the logic. Also, always use {} with if/else.
>
>> +  if (CurrentMediaId > Media->MediaId)
>> +    Media->MediaId = ++CurrentMediaId;
>> +  else
>> +    CurrentMediaId = Media->MediaId;
>
> I will give my interpretation of how this works, and you can confirm
> or deny it.
>
> When this Media entity is created, it is based on mMmcMediaTemplate.
> mMmcMediaTemplate (ab)uses the MediaId field as a Signature - "mmco",
> meaning any new MMC device starts with a MediaId of 0x6d6d626f (or
> 0x6f626d6d if I got the endianness wrong).
>
> So the value is initialised, just to the same for every MMC media in
> the system.
>
> Since the module-global (m)CurrentMediaId is initialised to 0, the
> first time MmcIdentificationMode() is called for the first eMMC
> device, (m)CurrentMediaId will be set to 0x6d6d6270 (or 0x6f626d6e).
>
> Does this not leave it uninitialised for MMC and other media types?
>
> I guess it's not a huge deal if the MediaID clashes for different
> devices, because it is mainly meant to indicate a removable media has
> changed. But that also means there should not have been a problem in
> the first place.
>
> So can you describe in the commit message what failure mode this patch
> gets rid of?
>

System won't crash with the patch removed. Let's remove this patch first.

Best Regards
Haojian

> Regardless, I think the code would be more clear in what it is doing
> if it did:
>   if (Media->MediaId == SIGNATURE_32('m','m','c','o')) {
>     Media->MediaId = ++CurrentMediaId;
>   } else {
>     CurrentMediaId = Media->MediaId;
>   }
>


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2016-11-13  6:46 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-08 15:21 [PATCH v4 00/11] enhance MMC Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 01/11] MmcDxe: wait OCR busy bit free Haojian Zhuang
2016-11-08 19:27   ` Leif Lindholm
2016-11-08 15:21 ` [PATCH v4 02/11] MmcDxe: move ECSD into CardInfo structure Haojian Zhuang
2016-11-08 20:27   ` Leif Lindholm
2016-11-08 15:21 ` [PATCH v4 03/11] MmcDxe: add SPEC_VERS field in CSD structure Haojian Zhuang
2016-11-08 20:27   ` Leif Lindholm
2016-11-08 15:21 ` [PATCH v4 04/11] MmcDxe: add interface to change io width and speed Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 05/11] MmcDxe: declare ECSD structure Haojian Zhuang
2016-11-08 20:43   ` Leif Lindholm
2016-11-09  0:34     ` Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 06/11] MmcDxe: set io bus width before reading EXTCSD Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 07/11] MmcDxe: Fix uninitialized mediaid for SD Haojian Zhuang
2016-11-08 23:59   ` Leif Lindholm
2016-11-13  6:46     ` Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 08/11] MmcDxe: set iospeed and bus width in SD stack Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 09/11] MmcDxe: expand to support multiple blocks Haojian Zhuang
2016-11-09  0:14   ` Leif Lindholm
2016-11-08 15:21 ` [PATCH v4 10/11] PL180: update for indentifying SD Haojian Zhuang
2016-11-08 15:21 ` [PATCH v4 11/11] PL180: set variable length for CMD6 and ACMD51 Haojian Zhuang
2016-11-08 17:32 ` [PATCH v4 00/11] enhance MMC Ryan Harkin
2016-11-09  0:30   ` Haojian Zhuang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox