| Message ID | 1343417387-13953-10-git-send-email-i.mitsyanko@samsung.com |
|---|---|
| State | New |
| Headers | show |
Igor Mitsyanko <i.mitsyanko@samsung.com> 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 <i.mitsyanko@samsung.com> > --- > 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? > + .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 */
On 31 July 2012 10:45, Markus Armbruster <armbru@redhat.com> wrote: > Igor Mitsyanko <i.mitsyanko@samsung.com> writes: >> @@ -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? This particular case I think is "SD card controller model tried to do something wrong". > Same for all the other fprintf(stderr, ...) in this file. Various uses: * guest legitimately did something we feel like telling the user about (eg "Card force-erased by CMD42") * guest did something dubious but with defined semantics ("Unknown CMD", trying to do something when the card is locked) * guest did something legitimate but unimplemented * underlying block layer read/write failed (and we are about to throw away the error rather than reporting it anywhere else!) These would all be worth tidying up some day when we have a sensible logging infrastructure to tidy them up into. -- PMM
On 07/31/2012 01:45 PM, Markus Armbruster wrote: > Igor Mitsyanko <i.mitsyanko@samsung.com> 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 <i.mitsyanko@samsung.com> >> --- >> 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 */
Igor Mitsyanko <i.mitsyanko@samsung.com> writes: > On 07/31/2012 01:45 PM, Markus Armbruster wrote: >> Igor Mitsyanko <i.mitsyanko@samsung.com> 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 <i.mitsyanko@samsung.com> >>> --- >>> 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. I'm afraid the first few device models that don't connect to a qbus are bound to flush out a few bugs. Nevertheless, device models should be subtypes of TYPE_DEVICE, shouldn't they? Anthony? > 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. No, to make SD a sub of TYPE_DEVICE, we need to fix the qdev remaining bugs around qbus-less device-device connections. [...]
On 31 July 2012 16:29, Markus Armbruster <armbru@redhat.com> wrote: > Igor Mitsyanko <i.mitsyanko@samsung.com> writes: >> 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. > > I'm afraid the first few device models that don't connect to a qbus are > bound to flush out a few bugs. Nevertheless, device models should be > subtypes of TYPE_DEVICE, shouldn't they? Anthony? Sounds right to me. Added bonus, we can use nice APIs for declaring and setting properties (qdev_prop_set_*) rather than nasty ones (object_property-set_*) :-) -- PMM
On 07/31/2012 08:17 PM, Peter Maydell wrote: > On 31 July 2012 16:29, Markus Armbruster <armbru@redhat.com> wrote: >> Igor Mitsyanko <i.mitsyanko@samsung.com> writes: >>> 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. >> I'm afraid the first few device models that don't connect to a qbus are >> bound to flush out a few bugs. Nevertheless, device models should be >> subtypes of TYPE_DEVICE, shouldn't they? Anthony? > Sounds right to me. Added bonus, we can use nice APIs for declaring > and setting properties (qdev_prop_set_*) rather than nasty ones > (object_property-set_*) :-) > > -- PMM > Just to clarify things for myself, system bus is supposed to go away in the future, but nobody knows when exactly since many things in qemu seriously intertwined with it. Bussless devices are presumably supported, but that support uses a "hack" of tying bussless devices to main system bus so they could be visible to the rest of the system. You're proposing to derive TYPE_SD directly from TYPE_DEVICE without specifying a bus for it, incidentally fixing any existing bugs in current bussless devices support.
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"); 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, + .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; +} + +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 */
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 <i.mitsyanko@samsung.com> --- hw/sd.c | 56 ++++++++++++++++++++++++++++++++++++++-------------- hw/sd.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 100 insertions(+), 23 deletions(-)