From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60090) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwDkh-0001Jc-BF for qemu-devel@nongnu.org; Tue, 31 Jul 2012 10:48:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwDka-0008L0-HW for qemu-devel@nongnu.org; Tue, 31 Jul 2012 10:48:51 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:52998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwDka-0008Kd-8W for qemu-devel@nongnu.org; Tue, 31 Jul 2012 10:48:44 -0400 Received: from eusync3.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0M8100IUM55W8T70@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 31 Jul 2012 15:49:08 +0100 (BST) Received: from [106.109.9.180] by eusync3.samsung.com (Oracle Communications Messaging Server 7u4-23.01(7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0M8100DQU54VHJ30@eusync3.samsung.com> for qemu-devel@nongnu.org; Tue, 31 Jul 2012 15:48:41 +0100 (BST) Message-id: <5017F03F.2060503@samsung.com> Date: Tue, 31 Jul 2012 18:48:31 +0400 From: Igor Mitsyanko MIME-version: 1.0 References: <1343417387-13953-1-git-send-email-i.mitsyanko@samsung.com> <1343417387-13953-10-git-send-email-i.mitsyanko@samsung.com> <873948i7qs.fsf@blackfin.pond.sub.org> In-reply-to: <873948i7qs.fsf@blackfin.pond.sub.org> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 09/12] hw/sd.c: convert SD state to QOM object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, peter.maydell@linaro.org, benoit.canet@gmail.com, wdongxu@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com, e.voevodin@samsung.com, qemu-devel@nongnu.org, andrew.zaborowski@intel.com, kyungmin.park@samsung.com, pbonzini@redhat.com On 07/31/2012 01:45 PM, Markus Armbruster wrote: > Igor Mitsyanko writes: > >> A straightforward conversion of SD card implementation to a proper QEMU object. >> Wrapper functions were introduced for SDClass methods in order to avoid SD card >> users modification. Because of this, name change for several functions in hw/sd.c >> was required. >> >> Signed-off-by: Igor Mitsyanko >> --- >> hw/sd.c | 56 ++++++++++++++++++++++++++++++++++++++-------------- >> hw/sd.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------- >> 2 files changed, 100 insertions(+), 23 deletions(-) >> >> diff --git a/hw/sd.c b/hw/sd.c >> index f8ab045..fe2c138 100644 >> --- a/hw/sd.c >> +++ b/hw/sd.c >> @@ -75,6 +75,8 @@ enum { >> }; >> >> struct SDState { >> + Object parent_obj; >> + >> uint32_t mode; >> int32_t state; >> uint32_t ocr; >> @@ -489,11 +491,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_card(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; >> @@ -503,10 +502,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_callbacks(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_send_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_card_data(SDState *sd, uint8_t value) >> { >> int i; >> >> @@ -1510,7 +1508,7 @@ void sd_write_data(SDState *sd, uint8_t value) >> return; >> >> if (sd->state != sd_receivingdata_state) { >> - fprintf(stderr, "sd_write_data: not in Receiving-Data state\n"); >> + fprintf(stderr, "sd_write_card_data: not in Receiving-Data state\n"); > Outside this patch's scope, but here goes anyway: what kind of condition > is reported here? Programming error that should never happen? Guest > doing something weird? > > Same for all the other fprintf(stderr, ...) in this file. > >> return; >> } >> >> @@ -1621,12 +1619,12 @@ void sd_write_data(SDState *sd, uint8_t value) >> break; >> >> default: >> - fprintf(stderr, "sd_write_data: unknown command\n"); >> + fprintf(stderr, "sd_write_card_data: unknown command\n"); >> break; >> } >> } >> >> -uint8_t sd_read_data(SDState *sd) >> +static uint8_t sd_read_card_data(SDState *sd) >> { >> /* TODO: Append CRCs */ >> uint8_t ret; >> @@ -1636,7 +1634,7 @@ uint8_t sd_read_data(SDState *sd) >> return 0x00; >> >> if (sd->state != sd_sendingdata_state) { >> - fprintf(stderr, "sd_read_data: not in Sending-Data state\n"); >> + fprintf(stderr, "sd_read_card_data: not in Sending-Data state\n"); >> return 0x00; >> } >> >> @@ -1738,19 +1736,47 @@ uint8_t sd_read_data(SDState *sd) >> break; >> >> default: >> - fprintf(stderr, "sd_read_data: unknown command\n"); >> + fprintf(stderr, "sd_read_card_data: unknown command\n"); >> return 0x00; >> } >> >> return ret; >> } >> >> -bool sd_data_ready(SDState *sd) >> +static bool sd_is_data_ready(SDState *sd) >> { >> return sd->state == sd_sendingdata_state; >> } >> >> -void sd_enable(SDState *sd, bool enable) >> +static void sd_enable_card(SDState *sd, bool enable) >> { >> sd->enable = enable; >> } >> + >> +static void sd_class_init(ObjectClass *klass, void *data) >> +{ >> + SDClass *k = SD_CLASS(klass); >> + >> + k->init = sd_init_card; >> + k->set_cb = sd_set_callbacks; >> + k->do_command = sd_send_command; >> + k->data_ready = sd_is_data_ready; >> + k->read_data = sd_read_card_data; >> + k->write_data = sd_write_card_data; >> + k->enable = sd_enable_card; >> +} >> + >> +static const TypeInfo sd_type_info = { >> + .name = TYPE_SD_CARD, >> + .parent = TYPE_OBJECT, > Possibly ignorant question: why TYPE_OBJECT, not TYPE_DEVICE? QEMU requires all objects derived from TYPE_DEVICE to be connected to some bus, if no bus was specified in new object class description, QEMU practically assumes this object to be a sysbus device and connects it to main system bus. A while ago it wasn't even possible to create a class directly derived from DEVICE_CLASS without tying this class to some bus, QEMU would have abort() during initialization. Now, after "bus_info" member was removed from DeviceClass structure, it became possible, but still, it most definitely will cause errors because QEMU will assume such an object to be a SysBusDevice. For example, sysbus_dev_print() (called by "info qtree" monitor command) directly casts DeviceState object to SysBusDevice without checking if it is actually possible. My point is, to make SD of TYPE_DEVICE we need to implement SD bus. I have no idea what we need it for and what is it supposed to do, I think we can leave it for further improvement. >> + .instance_size = sizeof(SDState), >> + .class_init = sd_class_init, >> + .class_size = sizeof(SDClass) >> +}; >> + >> +static void sd_register_types(void) >> +{ >> + type_register_static(&sd_type_info); >> +} >> + >> +type_init(sd_register_types) >> diff --git a/hw/sd.h b/hw/sd.h >> index 4eb9679..f84e863 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,61 @@ 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) >> + >> +static inline SDState *sd_init(BlockDriverState *bs, bool is_spi) >> +{ >> + SDState *sd = SD_CARD(object_new(TYPE_SD_CARD)); >> + SD_GET_CLASS(sd)->init(sd, bs, is_spi); >> + return sd; > Shouldn't bs and spi be properties? Oh, that's coming in PATCH > 10-11/12. > >> +} >> + >> +static inline int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response) >> +{ >> + return SD_GET_CLASS(sd)->do_command(sd, req, response); >> +} >> + >> +static inline uint8_t sd_read_data(SDState *sd) >> +{ >> + return SD_GET_CLASS(sd)->read_data(sd); >> +} >> + >> +static inline void sd_write_data(SDState *sd, uint8_t value) >> +{ >> + SD_GET_CLASS(sd)->write_data(sd, value); >> +} >> + >> +static inline bool sd_data_ready(SDState *sd) >> +{ >> + return SD_GET_CLASS(sd)->data_ready(sd); >> +} >> + >> +static inline void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert) >> +{ >> + SD_GET_CLASS(sd)->set_cb(sd, readonly, insert); >> +} >> + >> +static inline void sd_enable(SDState *sd, bool enable) >> +{ >> + SD_GET_CLASS(sd)->enable(sd, enable); >> +} >> >> #endif /* __hw_sd_h */