On 12/26/2011 06:58 PM, Peter Maydell wrote:
On 26 December 2011 10:03, Mitsyanko Igor<i.mitsya...@samsung.com> wrote:
We couldn't properly implement save/restore functionality of SD host controllers
states without SD card's state VMStateDescription implementation. This patch
updates SD card emulation to support save/load of card's state.
Signed-off-by: Mitsyanko Igor<i.mitsya...@samsung.com>
Ah, cool. I'd had a quick go at adding save/restore to sd.c but
ran aground on the wp_groups array issue.
(cc'd Juan as vmstate/migration expert)
---
hw/milkymist-memcard.c | 2 +
hw/sd.c | 115 +++++++++++++++++++++++++++++++++---------------
hw/sd.h | 1 +
3 files changed, 82 insertions(+), 36 deletions(-)
diff --git a/hw/milkymist-memcard.c b/hw/milkymist-memcard.c
index 865a46c..6e8b0e4 100644
--- a/hw/milkymist-memcard.c
+++ b/hw/milkymist-memcard.c
@@ -266,6 +266,8 @@ static const VMStateDescription vmstate_milkymist_memcard =
{
.minimum_version_id = 1,
.minimum_version_id_old = 1,
.fields = (VMStateField[]) {
+ VMSTATE_STRUCT_POINTER(card, MilkymistMemcardState, sd_vmstate,
+ SDState *),
This doesn't seem like the right approach to me -- the migration state
for the controller shouldn't have to include all the state for the card.
Instead I think it would be better for sd.c to register a vmstate struct
(by calling vmstate_register in sd_init) and thus handle its own
saving/loading. Then if/when we make sd.c a proper qemu object we
won't need to change the migration state for all the controllers.
Good point, thanks.
VMSTATE_INT32(command_write_ptr, MilkymistMemcardState),
VMSTATE_INT32(response_read_ptr, MilkymistMemcardState),
VMSTATE_INT32(response_len, MilkymistMemcardState),
diff --git a/hw/sd.c b/hw/sd.c
index 07eb263..2b489d3 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];
@@ -81,22 +85,22 @@ 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 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];
- int spi;
- int current_cmd;
+ uint8_t spi;
+ 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!
*/
- int expecting_acmd;
- int blk_written;
+ bool expecting_acmd;
Why have you changed expecting_acmd and enable to bool, but spi
to a uint8_t and wp_groups[] to a uint8_t[] ? This isn't very
consistent. (I'm vaguely against bool because you then end up
making other changes to change '1' to 'true' and so on.)
I tried to preserve alignment and potentially avoid unnecessary memory
consumption in arrays since bool size is implementation defined,
otherwise I would have change everything to bool.
I'll convert everything to uint8_t next time, you're right, this'll make
things simpler.
+ uint32_t blk_written;
uint64_t data_start;
uint32_t data_offset;
uint8_t data[512];
@@ -105,7 +109,7 @@ struct SDState {
BlockDriverState *bdrv;
uint8_t *buf;
- int enable;
+ bool enable;
};
static void sd_set_mode(SDState *sd)
@@ -415,14 +419,14 @@ 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);
- memset(sd->function_group, 0, sizeof(int) * 6);
+ sd->wp_groups = (uint8_t *)g_malloc0(sect);
+ memset(sd->function_group, 0, 6);
Since you're touching this line anyway, can we have
sizeof(sd->function_group) rather than that hardcoded 6 ?
Sure.
--
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsya...@samsung.com
