Hi,

On 3/7/23 15:24, Cédric Le Goater wrote:
The parameters mimick a real 4GB eMMC, but it can be set to various
sizes. Initially from Vincent Palatin <vpala...@chromium.org>

Signed-off-by: Cédric Le Goater <c...@kaod.org>
---
  hw/sd/sdmmc-internal.h |  97 ++++++++++++++++++++++++++++++++++++
  include/hw/sd/sd.h     |   1 +
  hw/sd/sd.c             | 109 ++++++++++++++++++++++++++++++++++++++++-
  3 files changed, 206 insertions(+), 1 deletion(-)

First pass review, this will take time...

+static void mmc_set_ext_csd(SDState *sd, uint64_t size)
+{
+    uint32_t sectcount = size >> HWBLOCK_SHIFT;
+
+    memset(sd->ext_csd, 0, sizeof(sd->ext_csd));
+
+    sd->ext_csd[EXT_CSD_S_CMD_SET] = 0x1; /* supported command sets */
+    sd->ext_csd[EXT_CSD_HPI_FEATURES] = 0x3; /* HPI features  */
+    sd->ext_csd[EXT_CSD_BKOPS_SUPPORT] = 0x1; /* Background operations */
+    sd->ext_csd[241] = 0xA; /* 1st initialization time after partitioning */
+    sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */
+    sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure feature */

We do not support (and are not interested in) that. I'll use 0x0 for
"do not support".

+    sd->ext_csd[EXT_CSD_SEC_ERASE_MULT] = 0x96; /* Secure erase support */

This value is obsolete, so I'd use 0x0 to avoid confusions.

+    sd->ext_csd[EXT_CSD_SEC_TRIM_MULT] = 0x96; /* Secure TRIM multiplier */

Again, 0x0 for "not defined".

+    sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */
+    sd->ext_csd[EXT_CSD_BOOT_MULT] = 0x8; /* Boot partition size. 128KB unit */
+    sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */

16KB of super_page_size hmm. Simpler could be the underlying block
retrieved with bdrv_nb_sectors() or simply BDRV_SECTOR_SIZE (0x1).

+    sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit size */

2MB of erase size hmmm why not.

+    sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x1; /* HC erase timeout */

We don't implement timeout, can we use 0?

+    sd->ext_csd[EXT_CSD_REL_WR_SEC_C] = 0x1; /* Reliable write sector count */
+    sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x4; /* HC write protect group size 
*/
+    sd->ext_csd[EXT_CSD_S_C_VCC] = 0x8; /* Sleep current VCC  */
+    sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x7; /* Sleep current VCCQ */
+    sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x11; /* Sleep/Awake timeout */
+    sd->ext_csd[215] = (sectcount >> 24) & 0xff; /* Sector count */
+    sd->ext_csd[214] = (sectcount >> 16) & 0xff; /* ... */
+    sd->ext_csd[213] = (sectcount >> 8) & 0xff;  /* ... */
+    sd->ext_csd[EXT_CSD_SEC_CNT] = (sectcount & 0xff);       /* ... */
+    sd->ext_csd[210] = 0xa; /* Min write perf for 8bit@52Mhz */
+    sd->ext_csd[209] = 0xa; /* Min read perf for 8bit@52Mhz  */
+    sd->ext_csd[208] = 0xa; /* Min write perf for 4bit@52Mhz */
+    sd->ext_csd[207] = 0xa; /* Min read perf for 4bit@52Mhz */
+    sd->ext_csd[206] = 0xa; /* Min write perf for 4bit@26Mhz */
+    sd->ext_csd[205] = 0xa; /* Min read perf for 4bit@26Mhz */

Class B at 3MB/s. I suppose announcing up to J at 21MB/s is safe (0x46).

+    sd->ext_csd[EXT_CSD_PART_SWITCH_TIME] = 0x1;

SWITCH command isn't implemented so far. We could use 0x0 for "not
defined".

+    sd->ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] = 0x1;

Similarly, 0x0 for "undefined" is legal.

+    sd->ext_csd[EXT_CSD_CARD_TYPE] = 0x7;

You anounce dual data rate. Could we just use High-Speed mode (0x3)
to ease modelling?

+    sd->ext_csd[EXT_CSD_STRUCTURE] = 0x2;
+    sd->ext_csd[EXT_CSD_REV] = 0x5;

This is Revision 1.5 (for MMC v4.41)... The first QEMU implementation
was based on Revision 1.3 (for MMC v4.3) and I'm seeing some features
from Revision 1.6 (for MMC v4.5)...

Do we want to implement all of them? Since we are adding from
scratch, I suggest we directly start with v4.5 (0x6).

Note, EXT_CSD_BUS_WIDTH is not set (0x0) meaning 1-bit data bus.
I'd set it to 0x2 (8-bit):

       sd->ext_csd[EXT_CSD_BUS_WIDTH] = EXT_CSD_BUS_WIDTH_8_MASK;

+    sd->ext_csd[EXT_CSD_RPMB_MULT] = 0x1; /* RPMB size */
+    sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
+    sd->ext_csd[159] = 0x00; /* Max enhanced area size */
+    sd->ext_csd[158] = 0x00; /* ... */
+    sd->ext_csd[157] = 0xEC; /* ... */
+}


Reply via email to