[Qemu-devel] [PATCH 0/6] SD save/load support and SD qomification

[Qemu-devel] [PATCH 0/6] SD save/load support and SD qomification

[Igor Mitsyanko]

PATCH1 converts wp_groups member of SDState to bitfield significantly reducing
memory consumption.
PATCH2-4 convert binary variables to bool type.
PATCH5 adds save/load support for SDState, intermediate variable introduced in SDState
to hold size of wp_groups array.
PATCH6 converts SD state to QOM object. QOMified implementation doesn't have any
advantages over current one but it gives us enormous possibilities for further
improvements.

Igor Mitsyanko (6):
  hw/sd.c: convert wp_groups in SDState to bitfield
  hw/sd.c: convert binary variables to bool
  hw/sd.c: make sd_dataready() return bool
  hw/sd.c: make sd_wp_addr() return bool
  hw/sd.c: add SD card save/load support
  hw/sd.c: convert to QOM object

 hw/milkymist-memcard.c |   25 ++++---
 hw/omap_mmc.c          |   29 ++++---
 hw/pl181.c             |   14 ++--
 hw/pxa2xx_mmci.c       |   22 ++++--
 hw/sd.c                |  190 +++++++++++++++++++++++++++++++++---------------
 hw/sd.h                |   30 ++++++--
 hw/ssi-sd.c            |   12 ++-
 7 files changed, 214 insertions(+), 108 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH 1/6] hw/sd.c: convert wp_groups in SDState to bitfield

[Igor Mitsyanko]

Representing each group write protection flag with only one bit instead of int
variable significantly reduces memory consumption.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 07eb263..23e5f2f 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -81,7 +81,7 @@ struct SDState {
     uint8_t sd_status[64];
     uint32_t vhs;
     int wp_switch;
-    int *wp_groups;
+    uint8_t *wp_groups;
     uint64_t size;
     int blk_len;
     uint32_t erase_start;
@@ -415,7 +415,7 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
     if (sd->wp_groups)
         g_free(sd->wp_groups);
     sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : 0;
-    sd->wp_groups = (int *) g_malloc0(sizeof(int) * sect);
+    sd->wp_groups = (uint8_t *)g_malloc0((sect >> 3) + 1);
     memset(sd->function_group, 0, sizeof(int) * 6);
     sd->erase_start = 0;
     sd->erase_end = 0;
@@ -484,9 +484,11 @@ static void sd_erase(SDState *sd)
     sd->erase_end = 0;
     sd->csd[14] |= 0x40;
 
-    for (i = start; i <= end; i ++)
-        if (sd->wp_groups[i])
+    for (i = start; i <= end; i++) {
+        if (sd->wp_groups[i >> 3] & (1 << (i & 0x7))) {
             sd->card_status |= WP_ERASE_SKIP;
+        }
+    }
 }
 
 static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
@@ -496,9 +498,12 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
 
     wpnum = addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
 
-    for (i = 0; i < 32; i ++, wpnum ++, addr += WPGROUP_SIZE)
-        if (addr < sd->size && sd->wp_groups[wpnum])
+    for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
+        if (addr < sd->size &&
+                (sd->wp_groups[wpnum >> 3] & (1 << (wpnum & 0x7)))) {
             ret |= (1 << i);
+        }
+    }
 
     return ret;
 }
@@ -536,8 +541,9 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
 
 static inline int sd_wp_addr(SDState *sd, uint32_t addr)
 {
-    return sd->wp_groups[addr >>
-            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)];
+    unsigned int grp = addr >>
+            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
+    return sd->wp_groups[grp >> 3] & (1 << (grp & 0x7)) ? 1 : 0;
 }
 
 static void sd_lock_command(SDState *sd)
@@ -560,8 +566,8 @@ static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        memset(sd->wp_groups, 0, sizeof(int) * (sd->size >>
-                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)));
+        memset(sd->wp_groups, 0, 1 + (sd->size >>
+                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT + 3)));
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;
@@ -1007,8 +1013,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
             }
 
             sd->state = sd_programming_state;
-            sd->wp_groups[addr >> (HWBLOCK_SHIFT +
-                            SECTOR_SHIFT + WPGROUP_SHIFT)] = 1;
+            sd->wp_groups[addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT +
+                    WPGROUP_SHIFT + 3)] |= 1 << ((addr >> (HWBLOCK_SHIFT +
+                            SECTOR_SHIFT + WPGROUP_SHIFT)) & 0x7);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
@@ -1027,8 +1034,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
             }
 
             sd->state = sd_programming_state;
-            sd->wp_groups[addr >> (HWBLOCK_SHIFT +
-                            SECTOR_SHIFT + WPGROUP_SHIFT)] = 0;
+            sd->wp_groups[addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT +
+                    WPGROUP_SHIFT + 3)] &= ~(1 << ((addr >> (HWBLOCK_SHIFT +
+                            SECTOR_SHIFT + WPGROUP_SHIFT)) & 0x7));
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/6] hw/sd.c: convert binary variables to bool

[Igor Mitsyanko]

Several members of SDState have type int when they actually are binary variables.
Change type of these variables to bool to improve code readability. Change SD API
to be in consistency with new variables type.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   24 ++++++++++++------------
 hw/sd.h |    4 ++--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 23e5f2f..4c29907 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -80,7 +80,7 @@ struct SDState {
     uint32_t card_status;
     uint8_t sd_status[64];
     uint32_t vhs;
-    int wp_switch;
+    bool wp_switch;
     uint8_t *wp_groups;
     uint64_t size;
     int blk_len;
@@ -90,12 +90,12 @@ struct SDState {
     int pwd_len;
     int function_group[6];
 
-    int spi;
+    bool spi;
     int current_cmd;
     /* True if we will handle the next command as an ACMD. Note that this does
      * *not* track the APP_CMD status bit!
      */
-    int expecting_acmd;
+    bool expecting_acmd;
     int blk_written;
     uint64_t data_start;
     uint32_t data_offset;
@@ -105,7 +105,7 @@ struct SDState {
     BlockDriverState *bdrv;
     uint8_t *buf;
 
-    int enable;
+    bool enable;
 };
 
 static void sd_set_mode(SDState *sd)
@@ -414,7 +414,7 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
 
     if (sd->wp_groups)
         g_free(sd->wp_groups);
-    sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : 0;
+    sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : false;
     sd->wp_groups = (uint8_t *)g_malloc0((sect >> 3) + 1);
     memset(sd->function_group, 0, sizeof(int) * 6);
     sd->erase_start = 0;
@@ -422,7 +422,7 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
     sd->size = size;
     sd->blk_len = 0x200;
     sd->pwd_len = 0;
-    sd->expecting_acmd = 0;
+    sd->expecting_acmd = false;
 }
 
 static void sd_cardchange(void *opaque, bool load)
@@ -444,14 +444,14 @@ static const BlockDevOps sd_block_ops = {
    whether card should be in SSI or MMC/SD mode.  It is also up to the
    board to ensure that ssi transfers only occur when the chip select
    is asserted.  */
-SDState *sd_init(BlockDriverState *bs, int is_spi)
+SDState *sd_init(BlockDriverState *bs, bool is_spi)
 {
     SDState *sd;
 
     sd = (SDState *) g_malloc0(sizeof(SDState));
     sd->buf = qemu_blockalign(bs, 512);
     sd->spi = is_spi;
-    sd->enable = 1;
+    sd->enable = true;
     sd_reset(sd, bs);
     if (sd->bdrv) {
         bdrv_attach_dev_nofail(sd->bdrv, sd);
@@ -1133,7 +1133,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
         if (sd->rca != rca)
             return sd_r0;
 
-        sd->expecting_acmd = 1;
+        sd->expecting_acmd = true;
         sd->card_status |= APP_CMD;
         return sd_r1;
 
@@ -1315,7 +1315,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
     if (sd->card_status & CARD_IS_LOCKED) {
         if (!cmd_valid_while_locked(sd, req)) {
             sd->card_status |= ILLEGAL_COMMAND;
-            sd->expecting_acmd = 0;
+            sd->expecting_acmd = false;
             fprintf(stderr, "SD: Card is locked\n");
             rtype = sd_illegal;
             goto send_response;
@@ -1326,7 +1326,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
     sd_set_mode(sd);
 
     if (sd->expecting_acmd) {
-        sd->expecting_acmd = 0;
+        sd->expecting_acmd = false;
         rtype = sd_app_command(sd, *req);
     } else {
         rtype = sd_normal_command(sd, *req);
@@ -1712,7 +1712,7 @@ int sd_data_ready(SDState *sd)
     return sd->state == sd_sendingdata_state;
 }
 
-void sd_enable(SDState *sd, int enable)
+void sd_enable(SDState *sd, bool enable)
 {
     sd->enable = enable;
 }
diff --git a/hw/sd.h b/hw/sd.h
index ac4b7c4..d25342f 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -67,13 +67,13 @@ typedef struct {
 
 typedef struct SDState SDState;
 
-SDState *sd_init(BlockDriverState *bs, int is_spi);
+SDState *sd_init(BlockDriverState *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
                   uint8_t *response);
 void sd_write_data(SDState *sd, uint8_t value);
 uint8_t sd_read_data(SDState *sd);
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
 int sd_data_ready(SDState *sd);
-void sd_enable(SDState *sd, int enable);
+void sd_enable(SDState *sd, bool enable);
 
 #endif	/* __hw_sd_h */
-- 
1.7.4.1

[Qemu-devel] [PATCH 3/6] hw/sd.c: make sd_dataready() return bool

[Igor Mitsyanko]

For the sake of code clarity

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |    2 +-
 hw/sd.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 4c29907..338c125 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -1707,7 +1707,7 @@ uint8_t sd_read_data(SDState *sd)
     return ret;
 }
 
-int sd_data_ready(SDState *sd)
+bool sd_data_ready(SDState *sd)
 {
     return sd->state == sd_sendingdata_state;
 }
diff --git a/hw/sd.h b/hw/sd.h
index d25342f..4eb9679 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -73,7 +73,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
 void sd_write_data(SDState *sd, uint8_t value);
 uint8_t sd_read_data(SDState *sd);
 void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
-int sd_data_ready(SDState *sd);
+bool sd_data_ready(SDState *sd);
 void sd_enable(SDState *sd, bool enable);
 
 #endif	/* __hw_sd_h */
-- 
1.7.4.1

[Qemu-devel] [PATCH 4/6] hw/sd.c: make sd_wp_addr() return bool

[Igor Mitsyanko]

For the sake of code clarity

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 338c125..63e458f 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -539,11 +539,11 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
     sd->data[66] = crc & 0xff;
 }
 
-static inline int sd_wp_addr(SDState *sd, uint32_t addr)
+static inline bool sd_wp_addr(SDState *sd, uint32_t addr)
 {
     unsigned int grp = addr >>
             (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
-    return sd->wp_groups[grp >> 3] & (1 << (grp & 0x7)) ? 1 : 0;
+    return !!(sd->wp_groups[grp >> 3] & (1 << (grp & 0x7)));
 }
 
 static void sd_lock_command(SDState *sd)
-- 
1.7.4.1

[Qemu-devel] [PATCH 5/6] hw/sd.c: add SD card save/load support

[Igor Mitsyanko]

This patch updates SD card emulation to support save/load of card's state.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   90 ++++++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 64 insertions(+), 26 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 63e458f..20e10bb 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -54,24 +54,28 @@ typedef enum {
     sd_illegal = -2,
 } sd_rsp_type_t;
 
+enum {
+    sd_inactive,
+    sd_card_identification_mode,
+    sd_data_transfer_mode,
+};
+
+enum {
+    sd_inactive_state = -1,
+    sd_idle_state = 0,
+    sd_ready_state,
+    sd_identification_state,
+    sd_standby_state,
+    sd_transfer_state,
+    sd_sendingdata_state,
+    sd_receivingdata_state,
+    sd_programming_state,
+    sd_disconnect_state,
+};
+
 struct SDState {
-    enum {
-        sd_inactive,
-        sd_card_identification_mode,
-        sd_data_transfer_mode,
-    } mode;
-    enum {
-        sd_inactive_state = -1,
-        sd_idle_state = 0,
-        sd_ready_state,
-        sd_identification_state,
-        sd_standby_state,
-        sd_transfer_state,
-        sd_sendingdata_state,
-        sd_receivingdata_state,
-        sd_programming_state,
-        sd_disconnect_state,
-    } state;
+    uint32_t mode;
+    int32_t state;
     uint32_t ocr;
     uint8_t scr[8];
     uint8_t cid[16];
@@ -82,21 +86,22 @@ struct SDState {
     uint32_t vhs;
     bool wp_switch;
     uint8_t *wp_groups;
+    uint32_t wp_groups_size;
     uint64_t size;
-    int blk_len;
+    uint32_t blk_len;
     uint32_t erase_start;
     uint32_t erase_end;
     uint8_t pwd[16];
-    int pwd_len;
-    int function_group[6];
+    uint32_t pwd_len;
+    uint8_t function_group[6];
 
     bool spi;
-    int current_cmd;
+    uint8_t current_cmd;
     /* True if we will handle the next command as an ACMD. Note that this does
      * *not* track the APP_CMD status bit!
      */
     bool expecting_acmd;
-    int blk_written;
+    uint32_t blk_written;
     uint64_t data_start;
     uint32_t data_offset;
     uint8_t data[512];
@@ -415,8 +420,9 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
     if (sd->wp_groups)
         g_free(sd->wp_groups);
     sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : false;
-    sd->wp_groups = (uint8_t *)g_malloc0((sect >> 3) + 1);
-    memset(sd->function_group, 0, sizeof(int) * 6);
+    sd->wp_groups_size = (sect >> 3) + 1;
+    sd->wp_groups = (uint8_t *)g_malloc0(sd->wp_groups_size);
+    memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->size = size;
@@ -440,6 +446,38 @@ static const BlockDevOps sd_block_ops = {
     .change_media_cb = sd_cardchange,
 };
 
+static const VMStateDescription sd_vmstate = {
+    .name = "sd-card",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(mode, SDState),
+        VMSTATE_INT32(state, SDState),
+        VMSTATE_UINT8_ARRAY(cid, SDState, 16),
+        VMSTATE_UINT8_ARRAY(csd, SDState, 16),
+        VMSTATE_UINT16(rca, SDState),
+        VMSTATE_UINT32(card_status, SDState),
+        VMSTATE_PARTIAL_BUFFER(sd_status, SDState, 1),
+        VMSTATE_UINT32(vhs, SDState),
+        VMSTATE_VBUFFER_UINT32(wp_groups, SDState, 1, NULL, 0, wp_groups_size),
+        VMSTATE_UINT32(blk_len, SDState),
+        VMSTATE_UINT32(erase_start, SDState),
+        VMSTATE_UINT32(erase_end, SDState),
+        VMSTATE_UINT8_ARRAY(pwd, SDState, 16),
+        VMSTATE_UINT32(pwd_len, SDState),
+        VMSTATE_UINT8_ARRAY(function_group, SDState, 6),
+        VMSTATE_UINT8(current_cmd, SDState),
+        VMSTATE_BOOL(expecting_acmd, SDState),
+        VMSTATE_UINT32(blk_written, SDState),
+        VMSTATE_UINT64(data_start, SDState),
+        VMSTATE_UINT32(data_offset, SDState),
+        VMSTATE_UINT8_ARRAY(data, SDState, 512),
+        VMSTATE_BUFFER_UNSAFE(buf, SDState, 1, 512),
+        VMSTATE_BOOL(enable, SDState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /* We do not model the chip select pin, so allow the board to select
    whether card should be in SSI or MMC/SD mode.  It is also up to the
    board to ensure that ssi transfers only occur when the chip select
@@ -457,6 +495,7 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
         bdrv_attach_dev_nofail(sd->bdrv, sd);
         bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
     }
+    vmstate_register(NULL, -1, &sd_vmstate, sd);
     return sd;
 }
 
@@ -566,8 +605,7 @@ static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        memset(sd->wp_groups, 0, 1 + (sd->size >>
-                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT + 3)));
+        memset(sd->wp_groups, 0, sd->wp_groups_size);
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;
-- 
1.7.4.1

[Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Igor Mitsyanko]

A straightforward conversion of SD card implementation to a proper QEMU object.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/milkymist-memcard.c |   25 +++++++++++++++----------
 hw/omap_mmc.c          |   29 +++++++++++++++++------------
 hw/pl181.c             |   14 ++++++++------
 hw/pxa2xx_mmci.c       |   22 ++++++++++++++--------
 hw/sd.c                |   48 +++++++++++++++++++++++++++++++++++++-----------
 hw/sd.h                |   30 ++++++++++++++++++++++--------
 hw/ssi-sd.c            |   12 +++++++-----
 7 files changed, 120 insertions(+), 60 deletions(-)

diff --git a/hw/milkymist-memcard.c b/hw/milkymist-memcard.c
index 3515c3c..8a2b9b8 100644
--- a/hw/milkymist-memcard.c
+++ b/hw/milkymist-memcard.c
@@ -97,7 +97,8 @@ static void memcard_sd_command(MilkymistMemcardState *s)
     req.crc = s->command[5];
 
     s->response[0] = req.cmd;
-    s->response_len = sd_do_command(s->card, &req, s->response+1);
+    s->response_len =
+        SD_GET_CLASS(s->card)->do_command(s->card, &req, s->response + 1);
     s->response_read_ptr = 0;
 
     if (s->response_len == 16) {
@@ -141,11 +142,12 @@ static uint64_t memcard_read(void *opaque, target_phys_addr_t addr,
         if (!s->enabled) {
             r = 0xffffffff;
         } else {
+            SDClass *sd_class = SD_GET_CLASS(s->card);
             r = 0;
-            r |= sd_read_data(s->card) << 24;
-            r |= sd_read_data(s->card) << 16;
-            r |= sd_read_data(s->card) << 8;
-            r |= sd_read_data(s->card);
+            r |= sd_class->read_data(s->card) << 24;
+            r |= sd_class->read_data(s->card) << 16;
+            r |= sd_class->read_data(s->card) << 8;
+            r |= sd_class->read_data(s->card);
         }
         break;
     case R_CLK2XDIV:
@@ -170,6 +172,7 @@ static void memcard_write(void *opaque, target_phys_addr_t addr, uint64_t value,
                           unsigned size)
 {
     MilkymistMemcardState *s = opaque;
+    SDClass *sd_class;
 
     trace_milkymist_memcard_memory_write(addr, value);
 
@@ -198,10 +201,11 @@ static void memcard_write(void *opaque, target_phys_addr_t addr, uint64_t value,
         if (!s->enabled) {
             break;
         }
-        sd_write_data(s->card, (value >> 24) & 0xff);
-        sd_write_data(s->card, (value >> 16) & 0xff);
-        sd_write_data(s->card, (value >> 8) & 0xff);
-        sd_write_data(s->card, value & 0xff);
+        sd_class = SD_GET_CLASS(s->card);
+        sd_class->write_data(s->card, (value >> 24) & 0xff);
+        sd_class->write_data(s->card, (value >> 16) & 0xff);
+        sd_class->write_data(s->card, (value >> 8) & 0xff);
+        sd_class->write_data(s->card, value & 0xff);
         break;
     case R_ENABLE:
         s->regs[addr] = value;
@@ -249,8 +253,9 @@ static int milkymist_memcard_init(SysBusDevice *dev)
     MilkymistMemcardState *s = FROM_SYSBUS(typeof(*s), dev);
     DriveInfo *dinfo;
 
+    s->card = SD_CARD(object_new(TYPE_SD_CARD));
     dinfo = drive_get_next(IF_SD);
-    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
+    SD_GET_CLASS(s->card)->init(s->card, dinfo ? dinfo->bdrv : NULL, false);
     s->enabled = dinfo ? bdrv_is_inserted(dinfo->bdrv) : 0;
 
     memory_region_init_io(&s->regs_region, &memcard_mmio_ops, s,
diff --git a/hw/omap_mmc.c b/hw/omap_mmc.c
index aec0285..a16762e 100644
--- a/hw/omap_mmc.c
+++ b/hw/omap_mmc.c
@@ -138,7 +138,8 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
     request.arg = host->arg;
     request.crc = 0; /* FIXME */
 
-    rsplen = sd_do_command(host->card, &request, response);
+    rsplen =
+        SD_GET_CLASS(host->card)->do_command(host->card, &request, response);
 
     /* TODO: validate CRCs */
     switch (resptype) {
@@ -219,6 +220,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
 
 static void omap_mmc_transfer(struct omap_mmc_s *host)
 {
+    SDClass *sd_class = SD_GET_CLASS(host->card);
     uint8_t value;
 
     if (!host->transfer)
@@ -229,10 +231,10 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
             if (host->fifo_len > host->af_level)
                 break;
 
-            value = sd_read_data(host->card);
+            value = sd_class->read_data(host->card);
             host->fifo[(host->fifo_start + host->fifo_len) & 31] = value;
             if (-- host->blen_counter) {
-                value = sd_read_data(host->card);
+                value = sd_class->read_data(host->card);
                 host->fifo[(host->fifo_start + host->fifo_len) & 31] |=
                         value << 8;
                 host->blen_counter --;
@@ -244,10 +246,10 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
                 break;
 
             value = host->fifo[host->fifo_start] & 0xff;
-            sd_write_data(host->card, value);
+            sd_class->write_data(host->card, value);
             if (-- host->blen_counter) {
                 value = host->fifo[host->fifo_start] >> 8;
-                sd_write_data(host->card, value);
+                sd_class->write_data(host->card, value);
                 host->blen_counter --;
             }
 
@@ -592,7 +594,8 @@ struct omap_mmc_s *omap_mmc_init(target_phys_addr_t base,
     memory_region_add_subregion(sysmem, base, &s->iomem);
 
     /* Instantiate the storage */
-    s->card = sd_init(bd, 0);
+    s->card = SD_CARD(object_new(TYPE_SD_CARD));
+    SD_GET_CLASS(s->card)->init(s->card, bd, false);
 
     return s;
 }
@@ -617,10 +620,11 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
     omap_l4_attach(ta, 0, &s->iomem);
 
     /* Instantiate the storage */
-    s->card = sd_init(bd, 0);
+    s->card = SD_CARD(object_new(TYPE_SD_CARD));
+    SD_GET_CLASS(s->card)->init(s->card, bd, false);
 
     s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
-    sd_set_cb(s->card, NULL, s->cdet);
+    SD_GET_CLASS(s->card)->set_cb(s->card, NULL, s->cdet);
 
     return s;
 }
@@ -628,14 +632,15 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
 void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover)
 {
     if (s->cdet) {
-        sd_set_cb(s->card, ro, s->cdet);
+        SD_GET_CLASS(s->card)->set_cb(s->card, ro, s->cdet);
         s->coverswitch = cover;
         qemu_set_irq(cover, s->cdet_state);
-    } else
-        sd_set_cb(s->card, ro, cover);
+    } else {
+        SD_GET_CLASS(s->card)->set_cb(s->card, ro, cover);
+    }
 }
 
 void omap_mmc_enable(struct omap_mmc_s *s, int enable)
 {
-    sd_enable(s->card, enable);
+    SD_GET_CLASS(s->card)->enable(s->card, !!enable);
 }
diff --git a/hw/pl181.c b/hw/pl181.c
index 7d91fbb..97d7d97 100644
--- a/hw/pl181.c
+++ b/hw/pl181.c
@@ -171,7 +171,7 @@ static void pl181_send_command(pl181_state *s)
     request.cmd = s->cmd & PL181_CMD_INDEX;
     request.arg = s->cmdarg;
     DPRINTF("Command %d %08x\n", request.cmd, request.arg);
-    rlen = sd_do_command(s->card, &request, response);
+    rlen = SD_GET_CLASS(s->card)->do_command(s->card, &request, response);
     if (rlen < 0)
         goto error;
     if (s->cmd & PL181_CMD_RESPONSE) {
@@ -209,18 +209,19 @@ error:
 
 static void pl181_fifo_run(pl181_state *s)
 {
+    SDClass *sd_class = SD_GET_CLASS(s->card);
     uint32_t bits;
     uint32_t value = 0;
     int n;
     int is_read;
 
     is_read = (s->datactrl & PL181_DATA_DIRECTION) != 0;
-    if (s->datacnt != 0 && (!is_read || sd_data_ready(s->card))
+    if (s->datacnt != 0 && (!is_read || sd_class->data_ready(s->card))
             && !s->linux_hack) {
         if (is_read) {
             n = 0;
             while (s->datacnt && s->fifo_len < PL181_FIFO_LEN) {
-                value |= (uint32_t)sd_read_data(s->card) << (n * 8);
+                value |= (uint32_t)sd_class->read_data(s->card) << (n * 8);
                 s->datacnt--;
                 n++;
                 if (n == 4) {
@@ -241,7 +242,7 @@ static void pl181_fifo_run(pl181_state *s)
                 }
                 n--;
                 s->datacnt--;
-                sd_write_data(s->card, value & 0xff);
+                sd_class->write_data(s->card, value & 0xff);
                 value >>= 8;
             }
         }
@@ -469,7 +470,7 @@ static void pl181_reset(DeviceState *d)
     s->mask[1] = 0;
 
     /* We can assume our GPIO outputs have been wired up now */
-    sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
+    SD_GET_CLASS(s->card)->set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
 }
 
 static int pl181_init(SysBusDevice *dev)
@@ -483,7 +484,8 @@ static int pl181_init(SysBusDevice *dev)
     sysbus_init_irq(dev, &s->irq[1]);
     qdev_init_gpio_out(&s->busdev.qdev, s->cardstatus, 2);
     dinfo = drive_get_next(IF_SD);
-    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
+    s->card = SD_CARD(object_new(TYPE_SD_CARD));
+    SD_GET_CLASS(s->card)->init(s->card, dinfo ? dinfo->bdrv : NULL, false);
     return 0;
 }
 
diff --git a/hw/pxa2xx_mmci.c b/hw/pxa2xx_mmci.c
index b505a4c..ef796b1 100644
--- a/hw/pxa2xx_mmci.c
+++ b/hw/pxa2xx_mmci.c
@@ -117,25 +117,30 @@ static void pxa2xx_mmci_int_update(PXA2xxMMCIState *s)
 
 static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 {
-    if (!s->active)
+    SDClass *sd_class = SD_GET_CLASS(s->card);
+
+    if (!s->active) {
         return;
+    }
 
     if (s->cmdat & CMDAT_WR_RD) {
         while (s->bytesleft && s->tx_len) {
-            sd_write_data(s->card, s->tx_fifo[s->tx_start ++]);
+            sd_class->write_data(s->card, s->tx_fifo[s->tx_start++]);
             s->tx_start &= 0x1f;
             s->tx_len --;
             s->bytesleft --;
         }
-        if (s->bytesleft)
+        if (s->bytesleft) {
             s->intreq |= INT_TXFIFO_REQ;
-    } else
+        }
+    } else {
         while (s->bytesleft && s->rx_len < 32) {
             s->rx_fifo[(s->rx_start + (s->rx_len ++)) & 0x1f] =
-                sd_read_data(s->card);
+                sd_class->read_data(s->card);
             s->bytesleft --;
             s->intreq |= INT_RXFIFO_REQ;
         }
+    }
 
     if (!s->bytesleft) {
         s->active = 0;
@@ -166,7 +171,7 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
     request.arg = s->arg;
     request.crc = 0;	/* FIXME */
 
-    rsplen = sd_do_command(s->card, &request, response);
+    rsplen = SD_GET_CLASS(s->card)->do_command(s->card, &request, response);
     s->intreq |= INT_END_CMD;
 
     memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
@@ -538,7 +543,8 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
     memory_region_add_subregion(sysmem, base, &s->iomem);
 
     /* Instantiate the actual storage */
-    s->card = sd_init(bd, 0);
+    s->card = SD_CARD(object_new(TYPE_SD_CARD));
+    SD_GET_CLASS(s->card)->init(s->card, bd, false);
 
     register_savevm(NULL, "pxa2xx_mmci", 0, 0,
                     pxa2xx_mmci_save, pxa2xx_mmci_load, s);
@@ -549,5 +555,5 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
                 qemu_irq coverswitch)
 {
-    sd_set_cb(s->card, readonly, coverswitch);
+    SD_GET_CLASS(s->card)->set_cb(s->card, readonly, coverswitch);
 }
diff --git a/hw/sd.c b/hw/sd.c
index 20e10bb..1abd198 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -74,6 +74,8 @@ enum {
 };
 
 struct SDState {
+    Object parent_obj;
+
     uint32_t mode;
     int32_t state;
     uint32_t ocr;
@@ -482,11 +484,8 @@ static const VMStateDescription sd_vmstate = {
    whether card should be in SSI or MMC/SD mode.  It is also up to the
    board to ensure that ssi transfers only occur when the chip select
    is asserted.  */
-SDState *sd_init(BlockDriverState *bs, bool is_spi)
+static void sd_init(SDState *sd, BlockDriverState *bs, bool is_spi)
 {
-    SDState *sd;
-
-    sd = (SDState *) g_malloc0(sizeof(SDState));
     sd->buf = qemu_blockalign(bs, 512);
     sd->spi = is_spi;
     sd->enable = true;
@@ -496,10 +495,9 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
         bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
     }
     vmstate_register(NULL, -1, &sd_vmstate, sd);
-    return sd;
 }
 
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
+static void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
 {
     sd->readonly_cb = readonly;
     sd->inserted_cb = insert;
@@ -1334,7 +1332,7 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
     return sd_cmd_class[req->cmd] == 0 || sd_cmd_class[req->cmd] == 7;
 }
 
-int sd_do_command(SDState *sd, SDRequest *req,
+static int sd_do_command(SDState *sd, SDRequest *req,
                   uint8_t *response) {
     int last_state;
     sd_rsp_type_t rtype;
@@ -1502,7 +1500,7 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 #define APP_READ_BLOCK(a, len)	memset(sd->data, 0xec, len)
 #define APP_WRITE_BLOCK(a, len)
 
-void sd_write_data(SDState *sd, uint8_t value)
+static void sd_write_data(SDState *sd, uint8_t value)
 {
     int i;
 
@@ -1626,7 +1624,7 @@ void sd_write_data(SDState *sd, uint8_t value)
     }
 }
 
-uint8_t sd_read_data(SDState *sd)
+static uint8_t sd_read_data(SDState *sd)
 {
     /* TODO: Append CRCs */
     uint8_t ret;
@@ -1745,12 +1743,40 @@ uint8_t sd_read_data(SDState *sd)
     return ret;
 }
 
-bool sd_data_ready(SDState *sd)
+static bool sd_data_ready(SDState *sd)
 {
     return sd->state == sd_sendingdata_state;
 }
 
-void sd_enable(SDState *sd, bool enable)
+static void sd_enable(SDState *sd, bool enable)
 {
     sd->enable = enable;
 }
+
+static void sd_class_init(ObjectClass *class, void *data)
+{
+    SDClass *k = SD_CLASS(class);
+
+    k->init = sd_init;
+    k->set_cb = sd_set_cb;
+    k->do_command = sd_do_command;
+    k->data_ready = sd_data_ready;
+    k->read_data = sd_read_data;
+    k->write_data = sd_write_data;
+    k->enable = sd_enable;
+}
+
+static TypeInfo sd_type_info = {
+    .name = TYPE_SD_CARD,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(SDState),
+    .class_init = sd_class_init,
+    .class_size = sizeof(SDClass)
+};
+
+static void sd_register_type(void)
+{
+    type_register_static(&sd_type_info);
+}
+
+type_init(sd_register_type)
diff --git a/hw/sd.h b/hw/sd.h
index 4eb9679..63b8203 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -29,6 +29,9 @@
 #ifndef __hw_sd_h
 #define __hw_sd_h		1
 
+#include "qemu-common.h"
+#include "qemu/object.h"
+
 #define OUT_OF_RANGE		(1 << 31)
 #define ADDRESS_ERROR		(1 << 30)
 #define BLOCK_LEN_ERROR		(1 << 29)
@@ -67,13 +70,24 @@ typedef struct {
 
 typedef struct SDState SDState;
 
-SDState *sd_init(BlockDriverState *bs, bool is_spi);
-int sd_do_command(SDState *sd, SDRequest *req,
-                  uint8_t *response);
-void sd_write_data(SDState *sd, uint8_t value);
-uint8_t sd_read_data(SDState *sd);
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert);
-bool sd_data_ready(SDState *sd);
-void sd_enable(SDState *sd, bool enable);
+typedef struct SDClass {
+    ObjectClass parent_class;
+
+    void (*init)(SDState *sd, BlockDriverState *bs, bool is_spi);
+    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
+    void (*write_data)(SDState *sd, uint8_t value);
+    uint8_t (*read_data)(SDState *sd);
+    void (*set_cb)(SDState *sd, qemu_irq readonly, qemu_irq insert);
+    bool (*data_ready)(SDState *sd);
+    void (*enable)(SDState *sd, bool enable);
+} SDClass;
+
+#define TYPE_SD_CARD            "sd-card"
+#define SD_CARD(obj)            \
+     OBJECT_CHECK(SDState, (obj), TYPE_SD_CARD)
+#define SD_CLASS(klass)         \
+     OBJECT_CLASS_CHECK(SDClass, (klass), TYPE_SD_CARD)
+#define SD_GET_CLASS(obj)       \
+     OBJECT_GET_CLASS(SDClass, (obj), TYPE_SD_CARD)
 
 #endif	/* __hw_sd_h */
diff --git a/hw/ssi-sd.c b/hw/ssi-sd.c
index b519bdb..9d6f95d 100644
--- a/hw/ssi-sd.c
+++ b/hw/ssi-sd.c
@@ -94,7 +94,8 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
             request.arg = (s->cmdarg[0] << 24) | (s->cmdarg[1] << 16)
                            | (s->cmdarg[2] << 8) | s->cmdarg[3];
             DPRINTF("CMD%d arg 0x%08x\n", s->cmd, request.arg);
-            s->arglen = sd_do_command(s->sd, &request, longresp);
+            s->arglen =
+                SD_GET_CLASS(s->sd)->do_command(s->sd, &request, longresp);
             if (s->arglen <= 0) {
                 s->arglen = 1;
                 s->response[0] = 4;
@@ -171,7 +172,7 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
             DPRINTF("Response 0x%02x\n", s->response[s->response_pos]);
             return s->response[s->response_pos++];
         }
-        if (sd_data_ready(s->sd)) {
+        if (SD_GET_CLASS(s->sd)->data_ready(s->sd)) {
             DPRINTF("Data read\n");
             s->mode = SSI_SD_DATA_START;
         } else {
@@ -184,8 +185,8 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
         s->mode = SSI_SD_DATA_READ;
         return 0xfe;
     case SSI_SD_DATA_READ:
-        val = sd_read_data(s->sd);
-        if (!sd_data_ready(s->sd)) {
+        val = SD_GET_CLASS(s->sd)->read_data(s->sd);
+        if (!SD_GET_CLASS(s->sd)->data_ready(s->sd)) {
             DPRINTF("Data read end\n");
             s->mode = SSI_SD_CMD;
         }
@@ -239,7 +240,8 @@ static int ssi_sd_init(SSISlave *dev)
 
     s->mode = SSI_SD_CMD;
     dinfo = drive_get_next(IF_SD);
-    s->sd = sd_init(dinfo ? dinfo->bdrv : NULL, 1);
+    s->sd = SD_CARD(object_new(TYPE_SD_CARD));
+    SD_GET_CLASS(s->sd)->init(s->sd, dinfo ? dinfo->bdrv : NULL, true);
     register_savevm(&dev->qdev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
     return 0;
 }
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 1/6] hw/sd.c: convert wp_groups in SDState to bitfield

[Peter Maydell]

On 2 April 2012 15:28, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Representing each group write protection flag with only one bit instead of int
> variable significantly reduces memory consumption.

Can we use the bitmap.h functions here rather than doing things
by hand? (scattered examples below, you get the idea)

> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |   36 ++++++++++++++++++++++--------------
>  1 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index 07eb263..23e5f2f 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -81,7 +81,7 @@ struct SDState {
>     uint8_t sd_status[64];
>     uint32_t vhs;
>     int wp_switch;
> -    int *wp_groups;
> +    uint8_t *wp_groups;
>     uint64_t size;
>     int blk_len;
>     uint32_t erase_start;
> @@ -415,7 +415,7 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
>     if (sd->wp_groups)
>         g_free(sd->wp_groups);
>     sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : 0;
> -    sd->wp_groups = (int *) g_malloc0(sizeof(int) * sect);
> +    sd->wp_groups = (uint8_t *)g_malloc0((sect >> 3) + 1);

sd->wp_groups = bitmap_new(sect);

>     memset(sd->function_group, 0, sizeof(int) * 6);
>     sd->erase_start = 0;
>     sd->erase_end = 0;
> @@ -484,9 +484,11 @@ static void sd_erase(SDState *sd)
>     sd->erase_end = 0;
>     sd->csd[14] |= 0x40;
>
> -    for (i = start; i <= end; i ++)
> -        if (sd->wp_groups[i])
> +    for (i = start; i <= end; i++) {
> +        if (sd->wp_groups[i >> 3] & (1 << (i & 0x7))) {

if (test_bit(i, sd->wp_groups)) {

>             sd->card_status |= WP_ERASE_SKIP;
> +        }
> +    }
>  }
>
>  static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
> @@ -496,9 +498,12 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>
>     wpnum = addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
>
> -    for (i = 0; i < 32; i ++, wpnum ++, addr += WPGROUP_SIZE)
> -        if (addr < sd->size && sd->wp_groups[wpnum])
> +    for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
> +        if (addr < sd->size &&
> +                (sd->wp_groups[wpnum >> 3] & (1 << (wpnum & 0x7)))) {
>             ret |= (1 << i);
> +        }
> +    }
>
>     return ret;
>  }
> @@ -536,8 +541,9 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>
>  static inline int sd_wp_addr(SDState *sd, uint32_t addr)
>  {
> -    return sd->wp_groups[addr >>
> -            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)];
> +    unsigned int grp = addr >>
> +            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
> +    return sd->wp_groups[grp >> 3] & (1 << (grp & 0x7)) ? 1 : 0;
>  }
>
>  static void sd_lock_command(SDState *sd)
> @@ -560,8 +566,8 @@ static void sd_lock_command(SDState *sd)
>             sd->card_status |= LOCK_UNLOCK_FAILED;
>             return;
>         }
> -        memset(sd->wp_groups, 0, sizeof(int) * (sd->size >>
> -                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)));
> +        memset(sd->wp_groups, 0, 1 + (sd->size >>
> +                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT + 3)));

bitmap_zero(sd->wp_groups, number-of-bits-to-clear)

>         sd->csd[14] &= ~0x10;
>         sd->card_status &= ~CARD_IS_LOCKED;
>         sd->pwd_len = 0;
> @@ -1007,8 +1013,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>             }
>
>             sd->state = sd_programming_state;
> -            sd->wp_groups[addr >> (HWBLOCK_SHIFT +
> -                            SECTOR_SHIFT + WPGROUP_SHIFT)] = 1;
> +            sd->wp_groups[addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT +
> +                    WPGROUP_SHIFT + 3)] |= 1 << ((addr >> (HWBLOCK_SHIFT +
> +                            SECTOR_SHIFT + WPGROUP_SHIFT)) & 0x7);

set_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT), sd->wp_groups);

>             /* Bzzzzzzztt .... Operation complete.  */
>             sd->state = sd_transfer_state;
>             return sd_r1b;
> @@ -1027,8 +1034,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>             }
>
>             sd->state = sd_programming_state;
> -            sd->wp_groups[addr >> (HWBLOCK_SHIFT +
> -                            SECTOR_SHIFT + WPGROUP_SHIFT)] = 0;
> +            sd->wp_groups[addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT +
> +                    WPGROUP_SHIFT + 3)] &= ~(1 << ((addr >> (HWBLOCK_SHIFT +
> +                            SECTOR_SHIFT + WPGROUP_SHIFT)) & 0x7));
>             /* Bzzzzzzztt .... Operation complete.  */
>             sd->state = sd_transfer_state;
>             return sd_r1b;
> --
> 1.7.4.1
>

Re: [Qemu-devel] [PATCH 2/6] hw/sd.c: convert binary variables to bool

[Peter Maydell]

On 2 April 2012 15:28, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Several members of SDState have type int when they actually are binary variables.
> Change type of these variables to bool to improve code readability. Change SD API
> to be in consistency with new variables type.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Re: [Qemu-devel] [PATCH 3/6] hw/sd.c: make sd_dataready() return bool

[Peter Maydell]

On 2 April 2012 15:28, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> For the sake of code clarity
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Peter Maydell]

On 2 April 2012 15:28, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> +    s->card = SD_CARD(object_new(TYPE_SD_CARD));
>     dinfo = drive_get_next(IF_SD);
> -    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
> +    SD_GET_CLASS(s->card)->init(s->card, dinfo ? dinfo->bdrv : NULL, false);

Ideally rather than having a class specific init function we
should make the 'is_spi' and 'bdrv' be object properties, and
then you do something like (syntax probably wrong but):
  s->card = SD_CARD(object_new(TYPE_SD_CARD));
  if (dinfo) {
     object_property_set_bdrv(s->card, dinfo->bdrv, "bdrv", errp);
  }
  realize(s->card);

(where the default for bdrv is NULL and the default for is_spi is false
so we don't need to set that).

This needs realize support for QOM objects, though.

-- PMM

Re: [Qemu-devel] [PATCH 5/6] hw/sd.c: add SD card save/load support

[Peter Maydell]

On 2 April 2012 15:28, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> This patch updates SD card emulation to support save/load of card's state.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Looks OK apart from the bits that will need to change to use the bitops/bitmap
functions (see earlier patch comments.)

-- PMM

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Paolo Bonzini]

Il 02/04/2012 18:48, Peter Maydell ha scritto:
> Ideally rather than having a class specific init function we
> should make the 'is_spi' and 'bdrv' be object properties, and
> then you do something like (syntax probably wrong but):
>   s->card = SD_CARD(object_new(TYPE_SD_CARD));
>   if (dinfo) {
>      object_property_set_bdrv(s->card, dinfo->bdrv, "bdrv", errp);

Long term, this would be a link property.  Short term it can be

object_property_set_str(s->card, bdrv_get_device_name(dinfo->bdrv),
                        "bdrv", errp);

>   }
>   realize(s->card);

No need to realize explicitly if you instead call
object_property_add_child to add s->card as a child of s.

> (where the default for bdrv is NULL and the default for is_spi is false
> so we don't need to set that).
> 
> This needs realize support for QOM objects, though.

And support for properties in non-device objects.  Both coming tomorrow. :)

Paolo

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Peter Maydell]

On 2 April 2012 21:56, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
> On 02.04.2012 7:48 PM, Peter Maydell wrote:
>> Ideally rather than having a class specific init function we
>> should make the 'is_spi' and 'bdrv' be object properties,
>>
>> This needs realize support for QOM objects, though.

> I absolutely agree, that's how I wanted to do this in my working tree at
> first, but then several issues arose:

I should mention that I don't want to make the perfect the
enemy of the good here -- I'd rather we commit something like
this now and improve it later than delay it waiting for
potential improvements to the QOM infrastructure...

-- PMM

Re: [Qemu-devel] [PATCH 1/6] hw/sd.c: convert wp_groups in SDState to bitfield

[Igor Mitsyanko]

On 02.04.2012 5:42 PM, Peter Maydell wrote:
> On 2 April 2012 15:28, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> Representing each group write protection flag with only one bit instead of int
>> variable significantly reduces memory consumption.
>
> Can we use the bitmap.h functions here rather than doing things
> by hand? (scattered examples below, you get the idea)
>

That's pretty cool, I didn't know about this, thanks.

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Igor Mitsyanko]

On 02.04.2012 7:48 PM, Peter Maydell wrote:
> On 2 April 2012 15:28, Igor Mitsyanko<i.mitsyanko@samsung.com>  wrote:
>> +    s->card = SD_CARD(object_new(TYPE_SD_CARD));
>>      dinfo = drive_get_next(IF_SD);
>> -    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
>> +    SD_GET_CLASS(s->card)->init(s->card, dinfo ? dinfo->bdrv : NULL, false);
>
> Ideally rather than having a class specific init function we
> should make the 'is_spi' and 'bdrv' be object properties, and
> then you do something like (syntax probably wrong but):
>    s->card = SD_CARD(object_new(TYPE_SD_CARD));
>    if (dinfo) {
>       object_property_set_bdrv(s->card, dinfo->bdrv, "bdrv", errp);
>    }
>    realize(s->card);
>
> (where the default for bdrv is NULL and the default for is_spi is false
> so we don't need to set that).
>
> This needs realize support for QOM objects, though.
>
> -- PMM
>
>

I absolutely agree, that's how I wanted to do this in my working tree at 
first, but then several issues arose:
1) Pointer properties are obviously interfere with QOM principles, BDRV 
should be a link instead;
2) At first I made SD card child of SD host controller, but it most 
certainly wrong, it should be a link. But where should it be placed, 
under /block-dev/* maybe?
3) There is no point in creating SD card object until user specified an 
image.
4) Not every host controller on specific board is actually active, and 
nothing good will happen if you will try to hot-insert SD card in 
controller which was not activated by kernel. Having a child<SD> 
property in every controller instance is wrong.

What I want to do now is introduce CardSlot object and probably also 
HostControllerClass. Boards will instantiate as much CardSlots as it 
actually has and connect them with SDHC with links. SDCard object will 
be dynamically created and destroyed by setting CardSlot::inserted and 
CardSlot::image properties, SDcards will be childs of CardSlots.
But I havn't figured out how to avoid additional overhead in such a scheme.

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Paolo Bonzini]

Il 02/04/2012 22:56, Igor Mitsyanko ha scritto:
> 2) At first I made SD card child of SD host controller, but it most
> certainly wrong, it should be a link.

This is a bit thorny, because BlockDriverState exposes a slot and its
medium, not just the medium (you can have an empty BDS, and that is not
NULL).

I think the right place for the SD card would be a child of the block
device, but as block devices are not qdevified, for now it can be left
as a child of the host controller.

The patch looks good, except that I would prefer to have wrappers for
SD_GET_CLASS(foo)->method(args) in sd.h.  It would also make the patch
much smaller.

Paolo

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Igor Mitsyanko]

On 04/03/2012 01:02 AM, Paolo Bonzini wrote:
> Il 02/04/2012 22:56, Igor Mitsyanko ha scritto:
>> 2) At first I made SD card child of SD host controller, but it most
>> certainly wrong, it should be a link.
>
> This is a bit thorny, because BlockDriverState exposes a slot and its
> medium, not just the medium (you can have an empty BDS, and that is not
> NULL).
>

I think there's no point in preserving BlockDriverState along with 
SDState when we eject image from slot. Just drive_add()-drive_init() it 
when user inserts image and drive_put_ref() when user ejects image.

> I think the right place for the SD card would be a child of the block
> device, but as block devices are not qdevified, for now it can be left
> as a child of the host controller.
>
QOM tree is intended for user, right?  As a user I would prefer to use 
"qom-set /my-board/slot0.image /home/dodo/my_sd.img" rather then 
"qom-set /my-board/cortex-a20/sdhc0/card.image /home/dod/my_sd.img".
Anyway, sdhc/child<card> is the best decision for now I think..

> The patch looks good, except that I would prefer to have wrappers for
> SD_GET_CLASS(foo)->method(args) in sd.h.  It would also make the patch
> much smaller.

OK, thanks.

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com

Re: [Qemu-devel] [PATCH 6/6] hw/sd.c: convert to QOM object

[Paolo Bonzini]

Il 03/04/2012 10:35, Igor Mitsyanko ha scritto:
> I think there's no point in preserving BlockDriverState along with
> SDState when we eject image from slot. Just drive_add()-drive_init() it
> when user inserts image and drive_put_ref() when user ejects image.

Note that the BlockDriverState currently cannot be changed without
deleting whatever device holds it (qdev properties can only be set at
construction time).

> As a user I would prefer to use
> "qom-set /my-board/slot0.image /home/dodo/my_sd.img" rather then
> "qom-set /my-board/cortex-a20/sdhc0/card.image /home/dod/my_sd.img".

You can use partial paths:

    qom-set sdhc0/card /home/dod/my_sd.img

More precisely, that would be something like

    # Add a block device pointing to the file
    blockdev-add my_sd file=/home/dod/my_sd.img

    # Point the drive property to it
    qom-set sdhc0/card drive=my_sd

The alternative would be something like this:

    # Add a block device pointing to the file
    blockdev-add my_sd file=/home/dod/my_sd.img

    # Make it visible as an SD card
    #   my_sd = parent path
    #   card = property name
    #   sd = class name
    qom-add my_sd card sd

    # Point the host controller to the newly-created card
    qom-set sdhc0 card=my_sd/card

> Anyway, sdhc/child<card> is the best decision for now I think..

Yes, agreed.

Paolo