From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RfVCJ-0008Dj-Gb for qemu-devel@nongnu.org; Tue, 27 Dec 2011 06:28:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RfVCI-0002dx-6c for qemu-devel@nongnu.org; Tue, 27 Dec 2011 06:27:59 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:42860) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RfVCH-0002dn-Qx for qemu-devel@nongnu.org; Tue, 27 Dec 2011 06:27:58 -0500 MIME-version: 1.0 Content-transfer-encoding: 7BIT Content-type: text/plain; charset=UTF-8; format=flowed Received: from euspt2 ([210.118.77.14]) by mailout4.w1.samsung.com (Sun Java(tm) System Messaging Server 6.3-8.04 (built Jul 29 2009; 32bit)) with ESMTP id <0LWV000U016GF930@mailout4.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 27 Dec 2011 11:27:52 +0000 (GMT) Received: from [106.109.8.52] by spt2.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0LWV00IF316GZL@spt2.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 27 Dec 2011 11:27:52 +0000 (GMT) Date: Tue, 27 Dec 2011 15:27:51 +0400 From: Mitsyanko Igor In-reply-to: Message-id: <4EF9ABB7.2060103@samsung.com> References: <1324893824-13558-1-git-send-email-i.mitsyanko@samsung.com> <1324893824-13558-3-git-send-email-i.mitsyanko@samsung.com> Subject: Re: [Qemu-devel] [PATCH 2/3] hw/sd.c: add SD card save/load support Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Juan Quintela On 12/26/2011 06:58 PM, Peter Maydell wrote: > On 26 December 2011 10:03, Mitsyanko Igor 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 > > 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.mitsyanko@samsung.com