Re: [Qemu-devel] [PATCH V4 09/12] hw/sd.c: convert SD state to QOM object

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

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 */