[Qemu-devel] [PATCH V4 00/12] SD save/load support, SD qomification and bug fixes

[Qemu-devel] [PATCH V4 00/12] SD save/load support, SD qomification and bug fixes

[Igor Mitsyanko]

v3->v4
PATCH11: "spi" property now can be set until SD card is not attached to BlockDriverState,
not when card in idle state;
PATCH12: "device-id" property renamed to "drive", few code movements;
PATCH13 droped, its functionality is available in BlockDriverState;

v2->v3
Patchset modified in such a way that all existing SD card model users are left intact.
PATCH10 modifies hw/omap_mmc.c, hw/pl181.c and hw/pxa2xx_mmci.c but I'm not sure about
its necessity.

 - don't use BITS_TO_LONGS when operating with bitfields;
 - added wrapper for trnslating SD card addres to wp group number (new PATCH3);
 - made sd_wp_addr() to receive uint64_t address argument  (new PATCH2);
 - fix SD card erase function (new PATCH4)
 - introduced wrapper inline functions to left SD card users intact after SD qomification;
 - drop "image" property patch and introduce "device-id" SD card property instead. Property
   ties SD card with BlockDriverState of the same name (BlockDriverState must already exist).
 - introduce SD card "eject" property, it can be used to detach BlockDriverState from SD card.
 - droped patch which makes SD card a child of host controllers, I think it shoul be done  

v1->v2
PATCH1: use bitmap.h heder for bit operations.
PATCH6: use wrappers to call SDClass methods
Added 4 new patches:
PATCH7 and PATCH8 drope passing arguments to SDClass::init functions and replace them with
SD card object properties. SDClass::init is trivialized to realize().
PATCH10 adds "image" property to SD card objects. This property can be used for
hot-insertion/hot-cardchange in virtual SD card slot.

PATCH1 converts wp_groups member of SDState to bitfield significantly reducing
memory consumption.
PATCH2-4 convert binary variables to bool type.
PATCH5 adds save/load support for SDState, intermediate variable introduced in SDState
to hold size of wp_groups array.
PATCH6 converts SD state to QOM object. QOMified implementation doesn't have any
advantages over current one but it gives us enormous possibilities for further
improvements.

Igor Mitsyanko (12):
  hw/sd.c: convert wp_groups in SDState to bitfield
  hw/sd.c: make sd_wp_addr() accept 64 bit address argument
  hw/sd.c: introduce wrapper for conversion address to wp group
  hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase
  hw/sd.c: convert binary variables to bool
  hw/sd.c: make sd_dataready() return bool
  hw/sd.c: make sd_wp_addr() return bool
  hw/sd.c: add SD card save/load support
  hw/sd.c: convert SD state to QOM object
  SD card users: optimize access to SDClass methods
  SD card: introduce "spi" property for SD card objects
  hw/sd.c: introduce SD card "drive" property

 hw/omap_mmc.c    |    9 +-
 hw/pl181.c       |    7 +-
 hw/pxa2xx_mmci.c |    6 +-
 hw/sd.c          |  307 +++++++++++++++++++++++++++++++++++++++++-------------
 hw/sd.h          |   74 ++++++++++++--
 5 files changed, 312 insertions(+), 91 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH V4 01/12] hw/sd.c: convert wp_groups in SDState to bitfield

[Igor Mitsyanko]

Representing each group write protection flag with only one bit instead of int
variable significantly reduces memory consumption.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 07eb263..575b509 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -32,6 +32,7 @@
 #include "hw.h"
 #include "block.h"
 #include "sd.h"
+#include "bitmap.h"
 
 //#define DEBUG_SD 1
 
@@ -81,7 +82,7 @@ struct SDState {
     uint8_t sd_status[64];
     uint32_t vhs;
     int wp_switch;
-    int *wp_groups;
+    unsigned long *wp_groups;
     uint64_t size;
     int blk_len;
     uint32_t erase_start;
@@ -415,7 +416,7 @@ 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);
+    sd->wp_groups = bitmap_new(sect);
     memset(sd->function_group, 0, sizeof(int) * 6);
     sd->erase_start = 0;
     sd->erase_end = 0;
@@ -484,9 +485,11 @@ static void sd_erase(SDState *sd)
     sd->erase_end = 0;
     sd->csd[14] |= 0x40;
 
-    for (i = start; i <= end; i ++)
-        if (sd->wp_groups[i])
+    for (i = start; i <= end; i++) {
+        if (test_bit(i, sd->wp_groups)) {
             sd->card_status |= WP_ERASE_SKIP;
+        }
+    }
 }
 
 static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
@@ -496,9 +499,11 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
 
     wpnum = addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
 
-    for (i = 0; i < 32; i ++, wpnum ++, addr += WPGROUP_SIZE)
-        if (addr < sd->size && sd->wp_groups[wpnum])
+    for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
+        if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
             ret |= (1 << i);
+        }
+    }
 
     return ret;
 }
@@ -536,8 +541,8 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
 
 static inline int sd_wp_addr(SDState *sd, uint32_t addr)
 {
-    return sd->wp_groups[addr >>
-            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)];
+    return test_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
+            sd->wp_groups);
 }
 
 static void sd_lock_command(SDState *sd)
@@ -560,8 +565,8 @@ static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        memset(sd->wp_groups, 0, sizeof(int) * (sd->size >>
-                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)));
+        bitmap_zero(sd->wp_groups,
+            (sd->size >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)) + 1);
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;
@@ -1007,8 +1012,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
             }
 
             sd->state = sd_programming_state;
-            sd->wp_groups[addr >> (HWBLOCK_SHIFT +
-                            SECTOR_SHIFT + WPGROUP_SHIFT)] = 1;
+            set_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
+                    sd->wp_groups);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
@@ -1027,8 +1032,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
             }
 
             sd->state = sd_programming_state;
-            sd->wp_groups[addr >> (HWBLOCK_SHIFT +
-                            SECTOR_SHIFT + WPGROUP_SHIFT)] = 0;
+            clear_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
+                    sd->wp_groups);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 01/12] hw/sd.c: convert wp_groups in SDState to bitfield

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Representing each group write protection flag with only one bit instead of int
> variable significantly reduces memory consumption.

...and it looks much nicer too.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH V4 02/12] hw/sd.c: make sd_wp_addr() accept 64 bit address argument

[Igor Mitsyanko]

Currently sd_wp_addr() accepts 32 bit address arguments therefore implicitly
restricting SD card address range. Change address argument type to uint64_t.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 575b509..e24d04a 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -539,7 +539,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
     sd->data[66] = crc & 0xff;
 }
 
-static inline int sd_wp_addr(SDState *sd, uint32_t addr)
+static inline int sd_wp_addr(SDState *sd, uint64_t addr)
 {
     return test_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
             sd->wp_groups);
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 02/12] hw/sd.c: make sd_wp_addr() accept 64 bit address argument

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Currently sd_wp_addr() accepts 32 bit address arguments therefore implicitly
> restricting SD card address range. Change address argument type to uint64_t.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH V4 03/12] hw/sd.c: introduce wrapper for conversion address to wp group

[Igor Mitsyanko]

Add wrapper function sd_addr_to_wpnum() to replace long address-->wg_group
conversion line.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index e24d04a..d0674d5 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -388,6 +388,11 @@ static void sd_response_r7_make(SDState *sd, uint8_t *response)
     response[3] = (sd->vhs >>  0) & 0xff;
 }
 
+static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
+{
+    return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
+}
+
 static void sd_reset(SDState *sd, BlockDriverState *bdrv)
 {
     uint64_t size;
@@ -400,7 +405,7 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
     }
     size = sect << 9;
 
-    sect = (size >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)) + 1;
+    sect = sd_addr_to_wpnum(size) + 1;
 
     sd->state = sd_idle_state;
     sd->rca = 0x0000;
@@ -477,10 +482,8 @@ static void sd_erase(SDState *sd)
         return;
     }
 
-    start = sd->erase_start >>
-            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
-    end = sd->erase_end >>
-            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
+    start = sd_addr_to_wpnum(sd->erase_start);
+    end = sd_addr_to_wpnum(sd->erase_end);
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->csd[14] |= 0x40;
@@ -497,7 +500,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
     uint32_t i, wpnum;
     uint32_t ret = 0;
 
-    wpnum = addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
+    wpnum = sd_addr_to_wpnum(addr);
 
     for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
         if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
@@ -541,8 +544,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
 
 static inline int sd_wp_addr(SDState *sd, uint64_t addr)
 {
-    return test_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
-            sd->wp_groups);
+    return test_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
 }
 
 static void sd_lock_command(SDState *sd)
@@ -565,8 +567,7 @@ static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        bitmap_zero(sd->wp_groups,
-            (sd->size >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)) + 1);
+        bitmap_zero(sd->wp_groups, sd_addr_to_wpnum(sd->size) + 1);
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;
@@ -1012,8 +1013,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
             }
 
             sd->state = sd_programming_state;
-            set_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
-                    sd->wp_groups);
+            set_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
@@ -1032,8 +1032,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
             }
 
             sd->state = sd_programming_state;
-            clear_bit(addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
-                    sd->wp_groups);
+            clear_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 03/12] hw/sd.c: introduce wrapper for conversion address to wp group

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Add wrapper function sd_addr_to_wpnum() to replace long address-->wg_group
> conversion line.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH V4 04/12] hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase

[Igor Mitsyanko]

Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
and CMD33, we have to account for this.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index d0674d5..f7aa580 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
         return;
     }
 
-    start = sd_addr_to_wpnum(sd->erase_start);
-    end = sd_addr_to_wpnum(sd->erase_end);
+    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
+            (uint64_t)sd->erase_start * 512 : sd->erase_start);
+    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
+            (uint64_t)sd->erase_end * 512 : sd->erase_end);
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->csd[14] |= 0x40;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 04/12] hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase

[Markus Armbruster]

Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
> and CMD33, we have to account for this.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index d0674d5..f7aa580 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>          return;
>      }
>  
> -    start = sd_addr_to_wpnum(sd->erase_start);
> -    end = sd_addr_to_wpnum(sd->erase_end);
> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);
>      sd->erase_start = 0;
>      sd->erase_end = 0;
>      sd->csd[14] |= 0x40;

Is this a bug fix?

If yes, I recommend to state that clearly in the subject, say "hw/sd.c:
Fix erase for high capacity cards".

Re: [Qemu-devel] [PATCH V4 04/12] hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase

[Igor Mitsyanko]

On 07/31/2012 01:29 PM, Markus Armbruster wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>
>> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
>> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
>> and CMD33, we have to account for this.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index d0674d5..f7aa580 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>>           return;
>>       }
>>   
>> -    start = sd_addr_to_wpnum(sd->erase_start);
>> -    end = sd_addr_to_wpnum(sd->erase_end);
>> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
>> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);
>>       sd->erase_start = 0;
>>       sd->erase_end = 0;
>>       sd->csd[14] |= 0x40;
> Is this a bug fix?
>
> If yes, I recommend to state that clearly in the subject, say "hw/sd.c:
> Fix erase for high capacity cards".
>
Yes it is, I'll change subject in next version then.

Re: [Qemu-devel] [PATCH V4 04/12] hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
> and CMD33, we have to account for this.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index d0674d5..f7aa580 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>          return;
>      }
>
> -    start = sd_addr_to_wpnum(sd->erase_start);
> -    end = sd_addr_to_wpnum(sd->erase_end);
> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);

I think this would be a little clearer phrased as:

    uint64_t erase_start = sd->erase_start;
    uint64_t erase_end = sd->erase_end;

    [...]

    if (extract32(sd->ocr, 30, 1)) {
        /* High capacity memory card: erase units are 512 byte blocks */
        erase_start *= 512;
        erase_end *= 512;
    }

    start = sd_addr_to_wpnum(erase_start);
    end = sd_addr_to_wpnum(erase_end);

(I don't insist on the extract32() if you don't like it, but I
would like to avoid the repeated inline ?: ops.)

-- PMM

Re: [Qemu-devel] [PATCH V4 04/12] hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase

[Igor Mitsyanko]

On 07/31/2012 06:34 PM, Peter Maydell wrote:
> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> Standard capacity cards SDSC use byte unit address while SDHC and SDXC Cards use
>> block unit address (512 bytes) when setting ERASE_START and ERASE_END with CMD32
>> and CMD33, we have to account for this.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index d0674d5..f7aa580 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -482,8 +482,10 @@ static void sd_erase(SDState *sd)
>>           return;
>>       }
>>
>> -    start = sd_addr_to_wpnum(sd->erase_start);
>> -    end = sd_addr_to_wpnum(sd->erase_end);
>> +    start = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_start * 512 : sd->erase_start);
>> +    end = sd_addr_to_wpnum(sd->ocr & (1 << 30) ?
>> +            (uint64_t)sd->erase_end * 512 : sd->erase_end);
>
> I think this would be a little clearer phrased as:
>
>      uint64_t erase_start = sd->erase_start;
>      uint64_t erase_end = sd->erase_end;
>
>      [...]
>
>      if (extract32(sd->ocr, 30, 1)) {
>          /* High capacity memory card: erase units are 512 byte blocks */
>          erase_start *= 512;
>          erase_end *= 512;
>      }
>
>      start = sd_addr_to_wpnum(erase_start);
>      end = sd_addr_to_wpnum(erase_end);
>
> (I don't insist on the extract32() if you don't like it, but I
> would like to avoid the repeated inline ?: ops.)
>
> -- PMM
>
>

I like it, I extensively use your new fancy extract/deposit api in my 
local work.

[Qemu-devel] [PATCH V4 05/12] hw/sd.c: convert binary variables to bool

[Igor Mitsyanko]

Several members of SDState have type int when they actually are binary variables.
Change type of these variables to bool to improve code readability. Change SD API
to be in consistency with new variables type.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd.c |   24 ++++++++++++------------
 hw/sd.h |    4 ++--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index f7aa580..d4a8927 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -81,7 +81,7 @@ struct SDState {
     uint32_t card_status;
     uint8_t sd_status[64];
     uint32_t vhs;
-    int wp_switch;
+    bool wp_switch;
     unsigned long *wp_groups;
     uint64_t size;
     int blk_len;
@@ -91,12 +91,12 @@ struct SDState {
     int pwd_len;
     int function_group[6];
 
-    int spi;
+    bool spi;
     int 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;
+    bool expecting_acmd;
     int blk_written;
     uint64_t data_start;
     uint32_t data_offset;
@@ -106,7 +106,7 @@ struct SDState {
     BlockDriverState *bdrv;
     uint8_t *buf;
 
-    int enable;
+    bool enable;
 };
 
 static void sd_set_mode(SDState *sd)
@@ -420,7 +420,7 @@ 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_switch = bdrv ? bdrv_is_read_only(bdrv) : false;
     sd->wp_groups = bitmap_new(sect);
     memset(sd->function_group, 0, sizeof(int) * 6);
     sd->erase_start = 0;
@@ -428,7 +428,7 @@ static void sd_reset(SDState *sd, BlockDriverState *bdrv)
     sd->size = size;
     sd->blk_len = 0x200;
     sd->pwd_len = 0;
-    sd->expecting_acmd = 0;
+    sd->expecting_acmd = false;
 }
 
 static void sd_cardchange(void *opaque, bool load)
@@ -450,14 +450,14 @@ static const BlockDevOps sd_block_ops = {
    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, int is_spi)
+SDState *sd_init(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 = 1;
+    sd->enable = true;
     sd_reset(sd, bs);
     if (sd->bdrv) {
         bdrv_attach_dev_nofail(sd->bdrv, sd);
@@ -1131,7 +1131,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
         if (sd->rca != rca)
             return sd_r0;
 
-        sd->expecting_acmd = 1;
+        sd->expecting_acmd = true;
         sd->card_status |= APP_CMD;
         return sd_r1;
 
@@ -1313,7 +1313,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
     if (sd->card_status & CARD_IS_LOCKED) {
         if (!cmd_valid_while_locked(sd, req)) {
             sd->card_status |= ILLEGAL_COMMAND;
-            sd->expecting_acmd = 0;
+            sd->expecting_acmd = false;
             fprintf(stderr, "SD: Card is locked\n");
             rtype = sd_illegal;
             goto send_response;
@@ -1324,7 +1324,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
     sd_set_mode(sd);
 
     if (sd->expecting_acmd) {
-        sd->expecting_acmd = 0;
+        sd->expecting_acmd = false;
         rtype = sd_app_command(sd, *req);
     } else {
         rtype = sd_normal_command(sd, *req);
@@ -1710,7 +1710,7 @@ int sd_data_ready(SDState *sd)
     return sd->state == sd_sendingdata_state;
 }
 
-void sd_enable(SDState *sd, int enable)
+void sd_enable(SDState *sd, bool enable)
 {
     sd->enable = enable;
 }
diff --git a/hw/sd.h b/hw/sd.h
index ac4b7c4..d25342f 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -67,13 +67,13 @@ typedef struct {
 
 typedef struct SDState SDState;
 
-SDState *sd_init(BlockDriverState *bs, int is_spi);
+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);
 int sd_data_ready(SDState *sd);
-void sd_enable(SDState *sd, int enable);
+void sd_enable(SDState *sd, bool enable);
 
 #endif	/* __hw_sd_h */
-- 
1.7.5.4

[Qemu-devel] [PATCH V4 06/12] hw/sd.c: make sd_dataready() return bool

[Igor Mitsyanko]

For the sake of code clarity

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd.c |    2 +-
 hw/sd.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index d4a8927..a51d18d 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -1705,7 +1705,7 @@ uint8_t sd_read_data(SDState *sd)
     return ret;
 }
 
-int sd_data_ready(SDState *sd)
+bool sd_data_ready(SDState *sd)
 {
     return sd->state == sd_sendingdata_state;
 }
diff --git a/hw/sd.h b/hw/sd.h
index d25342f..4eb9679 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -73,7 +73,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
 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);
-int sd_data_ready(SDState *sd);
+bool sd_data_ready(SDState *sd);
 void sd_enable(SDState *sd, bool enable);
 
 #endif	/* __hw_sd_h */
-- 
1.7.5.4

[Qemu-devel] [PATCH V4 07/12] hw/sd.c: make sd_wp_addr() return bool

[Igor Mitsyanko]

For the sake of code clarity

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index a51d18d..20ebd8e 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -544,7 +544,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
     sd->data[66] = crc & 0xff;
 }
 
-static inline int sd_wp_addr(SDState *sd, uint64_t addr)
+static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
 {
     return test_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
 }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 07/12] hw/sd.c: make sd_wp_addr() return bool

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> For the sake of code clarity
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH V4 08/12] hw/sd.c: add SD card save/load support

[Igor Mitsyanko]

This patch updates SD card model to support save/load of card's state.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 20ebd8e..f8ab045 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -55,24 +55,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];
@@ -83,21 +87,22 @@ struct SDState {
     uint32_t vhs;
     bool wp_switch;
     unsigned long *wp_groups;
+    uint32_t wpgrps_size;
     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];
 
     bool spi;
-    int current_cmd;
+    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!
      */
     bool expecting_acmd;
-    int blk_written;
+    uint32_t blk_written;
     uint64_t data_start;
     uint32_t data_offset;
     uint8_t data[512];
@@ -421,8 +426,9 @@ 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) : false;
+    sd->wpgrps_size = sect;
     sd->wp_groups = bitmap_new(sect);
-    memset(sd->function_group, 0, sizeof(int) * 6);
+    memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->size = size;
@@ -446,6 +452,39 @@ static const BlockDevOps sd_block_ops = {
     .change_media_cb = sd_cardchange,
 };
 
+static const VMStateDescription sd_vmstate = {
+    .name = "sd-card",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(mode, SDState),
+        VMSTATE_INT32(state, SDState),
+        VMSTATE_UINT8_ARRAY(cid, SDState, 16),
+        VMSTATE_UINT8_ARRAY(csd, SDState, 16),
+        VMSTATE_UINT16(rca, SDState),
+        VMSTATE_UINT32(card_status, SDState),
+        VMSTATE_PARTIAL_BUFFER(sd_status, SDState, 1),
+        VMSTATE_UINT32(vhs, SDState),
+        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0, wpgrps_size,
+                sizeof(unsigned long)),
+        VMSTATE_UINT32(blk_len, SDState),
+        VMSTATE_UINT32(erase_start, SDState),
+        VMSTATE_UINT32(erase_end, SDState),
+        VMSTATE_UINT8_ARRAY(pwd, SDState, 16),
+        VMSTATE_UINT32(pwd_len, SDState),
+        VMSTATE_UINT8_ARRAY(function_group, SDState, 6),
+        VMSTATE_UINT8(current_cmd, SDState),
+        VMSTATE_BOOL(expecting_acmd, SDState),
+        VMSTATE_UINT32(blk_written, SDState),
+        VMSTATE_UINT64(data_start, SDState),
+        VMSTATE_UINT32(data_offset, SDState),
+        VMSTATE_UINT8_ARRAY(data, SDState, 512),
+        VMSTATE_BUFFER_UNSAFE(buf, SDState, 1, 512),
+        VMSTATE_BOOL(enable, SDState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /* We do not model the chip select pin, so allow the board to select
    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
@@ -463,6 +502,7 @@ SDState *sd_init(BlockDriverState *bs, bool is_spi)
         bdrv_attach_dev_nofail(sd->bdrv, sd);
         bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
     }
+    vmstate_register(NULL, -1, &sd_vmstate, sd);
     return sd;
 }
 
@@ -569,7 +609,7 @@ static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        bitmap_zero(sd->wp_groups, sd_addr_to_wpnum(sd->size) + 1);
+        bitmap_zero(sd->wp_groups, sd->wpgrps_size);
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 08/12] hw/sd.c: add SD card save/load support

[Markus Armbruster]

Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> This patch updates SD card model to support save/load of card's state.
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> ---
>  hw/sd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 64 insertions(+), 24 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index 20ebd8e..f8ab045 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -55,24 +55,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;

Comment pointing to the related enum?

>      uint32_t ocr;
>      uint8_t scr[8];
>      uint8_t cid[16];

[...]

Re: [Qemu-devel] [PATCH V4 08/12] hw/sd.c: add SD card save/load support

[Igor Mitsyanko]

On 07/31/2012 01:33 PM, Markus Armbruster wrote:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
>
>> This patch updates SD card model to support save/load of card's state.
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++-----------------
>>   1 files changed, 64 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index 20ebd8e..f8ab045 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -55,24 +55,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;
> Comment pointing to the related enum?

Ok, I'll have to give them names then.

>>       uint32_t ocr;
>>       uint8_t scr[8];
>>       uint8_t cid[16];
> [...]
>

Re: [Qemu-devel] [PATCH V4 08/12] hw/sd.c: add SD card save/load support

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> +        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0, wpgrps_size,
> +                sizeof(unsigned long)),

Isn't this trying to use wpgrps_size as the number of unsigned longs in
the bitmap, when it's actually the size of the bitmap in bits?

(Does this correctly work in migration between 32 and 64 bit systems
where 'unsigned long' is a different size? How about between a little
endian 32 bit system and a big endian 64 bit system? I don't know
enough about the vmstate macros to be confident here...)

-- PMM

Re: [Qemu-devel] [PATCH V4 08/12] hw/sd.c: add SD card save/load support

[Igor Mitsyanko]

On 07/31/2012 06:56 PM, Peter Maydell wrote:
> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> +        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0, wpgrps_size,
>> +                sizeof(unsigned long)),
>
> Isn't this trying to use wpgrps_size as the number of unsigned longs in
> the bitmap, when it's actually the size of the bitmap in bits?
>
> (Does this correctly work in migration between 32 and 64 bit systems
> where 'unsigned long' is a different size? How about between a little
> endian 32 bit system and a big endian 64 bit system? I don't know
> enough about the vmstate macros to be confident here...)
>
> -- PMM
>
>

You're right, bitmap_new() can allocated buffers of different size for 
the same number of bits but different sizeof(long) value. Maybe always 
align allocated buffer size to 8 byte? Endianess seems like even bigger 
issue.. Looks like we need to think of something else here.

Re: [Qemu-devel] [PATCH V4 08/12] hw/sd.c: add SD card save/load support

[Peter Maydell]

On 31 July 2012 19:18, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 07/31/2012 06:56 PM, Peter Maydell wrote:
>> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>
>>> +        VMSTATE_BUFFER_MULTIPLY(wp_groups, SDState, 1, NULL, 0,
>>> wpgrps_size,
>>> +                sizeof(unsigned long)),
>>
>>
>> Isn't this trying to use wpgrps_size as the number of unsigned longs in
>> the bitmap, when it's actually the size of the bitmap in bits?
>>
>> (Does this correctly work in migration between 32 and 64 bit systems
>> where 'unsigned long' is a different size? How about between a little
>> endian 32 bit system and a big endian 64 bit system? I don't know
>> enough about the vmstate macros to be confident here...)

> You're right, bitmap_new() can allocated buffers of different size for the
> same number of bits but different sizeof(long) value. Maybe always align
> allocated buffer size to 8 byte? Endianess seems like even bigger issue..
> Looks like we need to think of something else here.

Having talked with Juan on IRC, I think the right thing here is to add
support for transmitting bitmaps to vmstate, so we can say
   VMSTATE_BITMAP(wp_groups, SDState, wpgrps_size)
and the vmstate code takes care of marshalling the bitmap to a standard
wire format (probably 32 bit little endian) and back.
I'll have a go at a patch...

-- PMM

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

[Igor Mitsyanko]

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");
         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 */
-- 
1.7.5.4

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

[Markus Armbruster]

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

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

[Peter Maydell]

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

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

[Igor Mitsyanko]

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

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

[Markus Armbruster]

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.

[...]

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

[Peter Maydell]

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

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

[Igor Mitsyanko]

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.

[Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Igor Mitsyanko]

Rather that repeatedly call SD_GET_CLASS() in a loop, call it once before
a loop starts.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/omap_mmc.c    |    9 +++++----
 hw/pl181.c       |    7 ++++---
 hw/pxa2xx_mmci.c |    6 ++++--
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/omap_mmc.c b/hw/omap_mmc.c
index aec0285..8ceb25b 100644
--- a/hw/omap_mmc.c
+++ b/hw/omap_mmc.c
@@ -219,6 +219,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
 
 static void omap_mmc_transfer(struct omap_mmc_s *host)
 {
+    SDClass *sd_class = SD_GET_CLASS(host->card);
     uint8_t value;
 
     if (!host->transfer)
@@ -229,10 +230,10 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
             if (host->fifo_len > host->af_level)
                 break;
 
-            value = sd_read_data(host->card);
+            value = sd_class->read_data(host->card);
             host->fifo[(host->fifo_start + host->fifo_len) & 31] = value;
             if (-- host->blen_counter) {
-                value = sd_read_data(host->card);
+                value = sd_class->read_data(host->card);
                 host->fifo[(host->fifo_start + host->fifo_len) & 31] |=
                         value << 8;
                 host->blen_counter --;
@@ -244,10 +245,10 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
                 break;
 
             value = host->fifo[host->fifo_start] & 0xff;
-            sd_write_data(host->card, value);
+            sd_class->write_data(host->card, value);
             if (-- host->blen_counter) {
                 value = host->fifo[host->fifo_start] >> 8;
-                sd_write_data(host->card, value);
+                sd_class->write_data(host->card, value);
                 host->blen_counter --;
             }
 
diff --git a/hw/pl181.c b/hw/pl181.c
index 7d91fbb..8cd898c 100644
--- a/hw/pl181.c
+++ b/hw/pl181.c
@@ -209,18 +209,19 @@ error:
 
 static void pl181_fifo_run(pl181_state *s)
 {
+    SDClass *sd_class = SD_GET_CLASS(s->card);
     uint32_t bits;
     uint32_t value = 0;
     int n;
     int is_read;
 
     is_read = (s->datactrl & PL181_DATA_DIRECTION) != 0;
-    if (s->datacnt != 0 && (!is_read || sd_data_ready(s->card))
+    if (s->datacnt != 0 && (!is_read || sd_class->data_ready(s->card))
             && !s->linux_hack) {
         if (is_read) {
             n = 0;
             while (s->datacnt && s->fifo_len < PL181_FIFO_LEN) {
-                value |= (uint32_t)sd_read_data(s->card) << (n * 8);
+                value |= (uint32_t)sd_class->read_data(s->card) << (n * 8);
                 s->datacnt--;
                 n++;
                 if (n == 4) {
@@ -241,7 +242,7 @@ static void pl181_fifo_run(pl181_state *s)
                 }
                 n--;
                 s->datacnt--;
-                sd_write_data(s->card, value & 0xff);
+                sd_class->write_data(s->card, value & 0xff);
                 value >>= 8;
             }
         }
diff --git a/hw/pxa2xx_mmci.c b/hw/pxa2xx_mmci.c
index b505a4c..af19c36 100644
--- a/hw/pxa2xx_mmci.c
+++ b/hw/pxa2xx_mmci.c
@@ -117,12 +117,14 @@ static void pxa2xx_mmci_int_update(PXA2xxMMCIState *s)
 
 static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 {
+    SDClass *sd_class = SD_GET_CLASS(s->card);
+
     if (!s->active)
         return;
 
     if (s->cmdat & CMDAT_WR_RD) {
         while (s->bytesleft && s->tx_len) {
-            sd_write_data(s->card, s->tx_fifo[s->tx_start ++]);
+            sd_class->write_data(s->card, s->tx_fifo[s->tx_start ++]);
             s->tx_start &= 0x1f;
             s->tx_len --;
             s->bytesleft --;
@@ -132,7 +134,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
     } else
         while (s->bytesleft && s->rx_len < 32) {
             s->rx_fifo[(s->rx_start + (s->rx_len ++)) & 0x1f] =
-                sd_read_data(s->card);
+                sd_class->read_data(s->card);
             s->bytesleft --;
             s->intreq |= INT_RXFIFO_REQ;
         }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Rather that repeatedly call SD_GET_CLASS() in a loop, call it once before
> a loop starts.

Anthony claims that SD_GET_CLASS should be cheap enough that we don't
need to hoist it out of loops like this. Do you have profiling data
or similar that caused you to write this patch?

-- PMM

Re: [Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Igor Mitsyanko]

On 07/31/2012 07:43 PM, Peter Maydell wrote:
> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> Rather that repeatedly call SD_GET_CLASS() in a loop, call it once before
>> a loop starts.
> Anthony claims that SD_GET_CLASS should be cheap enough that we don't
> need to hoist it out of loops like this. Do you have profiling data
> or similar that caused you to write this patch?
>
> -- PMM
>
Well, I've tested it by measuring an execution time of a 4Kb write to SD 
card, results showed that arithmetic mean of time for one 4k write was 
less by ~300us in sequence with SD_GET_CLASS extracted from the loop. 
Although I ran this test several times, I have little faith in test 
methodology and results, it obviously showed significant dispersion 
between measured time of distinct 4K writes (200-300% if I recall 
correctly). I really have no objection no objection to dropping this patch.

Re: [Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Peter Maydell]

On 31 July 2012 18:33, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 07/31/2012 07:43 PM, Peter Maydell wrote:
>> Anthony claims that SD_GET_CLASS should be cheap enough that we don't
>> need to hoist it out of loops like this. Do you have profiling data
>> or similar that caused you to write this patch?

> Well, I've tested it by measuring an execution time of a 4Kb write to SD
> card, results showed that arithmetic mean of time for one 4k write was less
> by ~300us in sequence with SD_GET_CLASS extracted from the loop.

How much is that as a % of the total time for the write ?

-- PMM

Re: [Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Igor Mitsyanko]

On 07/31/2012 09:47 PM, Peter Maydell wrote:
> On 31 July 2012 18:33, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> On 07/31/2012 07:43 PM, Peter Maydell wrote:
>>> Anthony claims that SD_GET_CLASS should be cheap enough that we don't
>>> need to hoist it out of loops like this. Do you have profiling data
>>> or similar that caused you to write this patch?
>> Well, I've tested it by measuring an execution time of a 4Kb write to SD
>> card, results showed that arithmetic mean of time for one 4k write was less
>> by ~300us in sequence with SD_GET_CLASS extracted from the loop.
> How much is that as a % of the total time for the write ?
>
> -- PMM
>
total write time was ~3.5-4.0 ms I think, so its ~0.08%

Re: [Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Anthony Liguori]

Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> On 07/31/2012 07:43 PM, Peter Maydell wrote:
>> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>> Rather that repeatedly call SD_GET_CLASS() in a loop, call it once before
>>> a loop starts.
>> Anthony claims that SD_GET_CLASS should be cheap enough that we don't
>> need to hoist it out of loops like this. Do you have profiling data
>> or similar that caused you to write this patch?
>>
>> -- PMM
>>
> Well, I've tested it by measuring an execution time of a 4Kb write to SD 
> card, results showed that arithmetic mean of time for one 4k write was 
> less by ~300us in sequence with SD_GET_CLASS extracted from the loop.

How many loop iterations that?  300us is a huge amount of time, unless
you were looping on every byte, I have a hard time understanding that
delta.

Regards,

Anthony Liguori

> Although I ran this test several times, I have little faith in test 
> methodology and results, it obviously showed significant dispersion 
> between measured time of distinct 4K writes (200-300% if I recall 
> correctly). I really have no objection no objection to dropping this patch.

Re: [Qemu-devel] [PATCH V4 10/12] SD card users: optimize access to SDClass methods

[Igor Mitsyanko]

On 07/31/2012 10:15 PM, Anthony Liguori wrote:
> How many loop iterations that?  300us is a huge amount of time, unless
> you were looping on every byte, I have a hard time understanding that
> delta.

I'm not sure I understand what you mean, I did loop on every byte, 
that's how our SD model works. So, for 4K bytes qemu have to call 
SD_GET_CLASS >4K times.
Or you're asking about write sequence length? It was 10 writes of 4096 
bytes (not statistically reliable, I have to agree).

[Qemu-devel] [PATCH V4 11/12] SD card: introduce "spi" property for SD card objects

[Igor Mitsyanko]

And drop passing is_spi argument to SDCardClass::init function.
"spi" property could be set only while SD card object is not
attached to any BlockDriverState.
It defaults to "false".

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Cc: Paul Brook <paul@codesourcery.com>
---
 hw/sd.c |   33 +++++++++++++++++++++++++++++++--
 hw/sd.h |    8 ++++++--
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index fe2c138..611e1f3 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -491,10 +491,9 @@ 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.  */
-static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
+static void sd_init_card(SDState *sd, BlockDriverState *bs)
 {
     sd->buf = qemu_blockalign(bs, 512);
-    sd->spi = is_spi;
     sd->enable = true;
     sd_reset(sd, bs);
     if (sd->bdrv) {
@@ -1766,10 +1765,40 @@ static void sd_class_init(ObjectClass *klass, void *data)
     k->enable = sd_enable_card;
 }
 
+static void sd_is_spi(Object *obj, Visitor *v, void *opaque,
+                         const char *name, Error **errp)
+{
+    SDState *sd = SD_CARD(obj);
+
+    visit_type_bool(v, &sd->spi, name, errp);
+}
+
+static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
+                         const char *name, Error **errp)
+{
+    SDState *sd = SD_CARD(obj);
+
+    if (sd->bdrv) {
+        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(sd->bdrv));
+    } else {
+        visit_type_bool(v, &sd->spi, name, errp);
+    }
+}
+
+static void sd_initfn(Object *obj)
+{
+    SDState *sd = SD_CARD(obj);
+
+    sd->spi = false;
+    object_property_add(obj, "spi", "boolean", sd_is_spi, sd_set_spimode,
+            NULL, NULL, NULL);
+}
+
 static const TypeInfo sd_type_info = {
     .name = TYPE_SD_CARD,
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(SDState),
+    .instance_init = sd_initfn,
     .class_init = sd_class_init,
     .class_size = sizeof(SDClass)
 };
diff --git a/hw/sd.h b/hw/sd.h
index f84e863..c78eaa1 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -31,6 +31,7 @@
 
 #include "qemu-common.h"
 #include "qemu/object.h"
+#include "qapi/qapi-visit-core.h"
 
 #define OUT_OF_RANGE		(1 << 31)
 #define ADDRESS_ERROR		(1 << 30)
@@ -73,7 +74,7 @@ typedef struct SDState SDState;
 typedef struct SDClass {
     ObjectClass parent_class;
 
-    void (*init)(SDState *sd, BlockDriverState *bs, bool is_spi);
+    void (*init)(SDState *sd, BlockDriverState *bdrv);
     int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
     void (*write_data)(SDState *sd, uint8_t value);
     uint8_t (*read_data)(SDState *sd);
@@ -93,7 +94,10 @@ typedef struct SDClass {
 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);
+    Error *errp = NULL;
+    object_property_set_bool(OBJECT(sd), is_spi, "spi", &errp);
+    assert_no_error(errp);
+    SD_GET_CLASS(sd)->init(sd, bs);
     return sd;
 }
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 11/12] SD card: introduce "spi" property for SD card objects

[Markus Armbruster]

Igor Mitsyanko <i.mitsyanko@samsung.com> writes:

> And drop passing is_spi argument to SDCardClass::init function.
> "spi" property could be set only while SD card object is not
> attached to any BlockDriverState.
> It defaults to "false".
>
> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> Cc: Paul Brook <paul@codesourcery.com>
> ---
>  hw/sd.c |   33 +++++++++++++++++++++++++++++++--
>  hw/sd.h |    8 ++++++--
>  2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index fe2c138..611e1f3 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -491,10 +491,9 @@ 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.  */
> -static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
> +static void sd_init_card(SDState *sd, BlockDriverState *bs)
>  {
>      sd->buf = qemu_blockalign(bs, 512);
> -    sd->spi = is_spi;
>      sd->enable = true;
>      sd_reset(sd, bs);
>      if (sd->bdrv) {
> @@ -1766,10 +1765,40 @@ static void sd_class_init(ObjectClass *klass, void *data)
>      k->enable = sd_enable_card;
>  }
>  
> +static void sd_is_spi(Object *obj, Visitor *v, void *opaque,
> +                         const char *name, Error **errp)
> +{
> +    SDState *sd = SD_CARD(obj);
> +
> +    visit_type_bool(v, &sd->spi, name, errp);
> +}
> +
> +static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
> +                         const char *name, Error **errp)
> +{
> +    SDState *sd = SD_CARD(obj);
> +
> +    if (sd->bdrv) {
> +        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(sd->bdrv));
> +    } else {
> +        visit_type_bool(v, &sd->spi, name, errp);
> +    }
> +}
> +
> +static void sd_initfn(Object *obj)
> +{
> +    SDState *sd = SD_CARD(obj);
> +
> +    sd->spi = false;
> +    object_property_add(obj, "spi", "boolean", sd_is_spi, sd_set_spimode,
> +            NULL, NULL, NULL);
> +}
> +
>  static const TypeInfo sd_type_info = {
>      .name = TYPE_SD_CARD,
>      .parent = TYPE_OBJECT,
>      .instance_size = sizeof(SDState),
> +    .instance_init = sd_initfn,
>      .class_init = sd_class_init,
>      .class_size = sizeof(SDClass)
>  };

I suspect this would be much simpler the declarative way qdevs normally
use.  For an example, check out scsi_hd_properties[] and its use in
hw/scsi-disk.c.

> diff --git a/hw/sd.h b/hw/sd.h
> index f84e863..c78eaa1 100644
> --- a/hw/sd.h
> +++ b/hw/sd.h
> @@ -31,6 +31,7 @@
>  
>  #include "qemu-common.h"
>  #include "qemu/object.h"
> +#include "qapi/qapi-visit-core.h"
>  
>  #define OUT_OF_RANGE		(1 << 31)
>  #define ADDRESS_ERROR		(1 << 30)
> @@ -73,7 +74,7 @@ typedef struct SDState SDState;
>  typedef struct SDClass {
>      ObjectClass parent_class;
>  
> -    void (*init)(SDState *sd, BlockDriverState *bs, bool is_spi);
> +    void (*init)(SDState *sd, BlockDriverState *bdrv);
>      int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
>      void (*write_data)(SDState *sd, uint8_t value);
>      uint8_t (*read_data)(SDState *sd);
> @@ -93,7 +94,10 @@ typedef struct SDClass {
>  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);
> +    Error *errp = NULL;
> +    object_property_set_bool(OBJECT(sd), is_spi, "spi", &errp);
> +    assert_no_error(errp);
> +    SD_GET_CLASS(sd)->init(sd, bs);
>      return sd;
>  }

Re: [Qemu-devel] [PATCH V4 11/12] SD card: introduce "spi" property for SD card objects

[Andreas Färber]

Am 31.07.2012 11:54, schrieb Markus Armbruster:
> Igor Mitsyanko <i.mitsyanko@samsung.com> writes:
> 
>> And drop passing is_spi argument to SDCardClass::init function.
>> "spi" property could be set only while SD card object is not
>> attached to any BlockDriverState.
>> It defaults to "false".
>>
>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
>> Cc: Paul Brook <paul@codesourcery.com>
>> ---
>>  hw/sd.c |   33 +++++++++++++++++++++++++++++++--
>>  hw/sd.h |    8 ++++++--
>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index fe2c138..611e1f3 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -491,10 +491,9 @@ 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.  */
>> -static void sd_init_card(SDState *sd, BlockDriverState *bs, bool is_spi)
>> +static void sd_init_card(SDState *sd, BlockDriverState *bs)
>>  {
>>      sd->buf = qemu_blockalign(bs, 512);
>> -    sd->spi = is_spi;
>>      sd->enable = true;
>>      sd_reset(sd, bs);
>>      if (sd->bdrv) {
>> @@ -1766,10 +1765,40 @@ static void sd_class_init(ObjectClass *klass, void *data)
>>      k->enable = sd_enable_card;
>>  }
>>  
>> +static void sd_is_spi(Object *obj, Visitor *v, void *opaque,
>> +                         const char *name, Error **errp)
>> +{
>> +    SDState *sd = SD_CARD(obj);
>> +
>> +    visit_type_bool(v, &sd->spi, name, errp);
>> +}
>> +
>> +static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
>> +                         const char *name, Error **errp)
>> +{
>> +    SDState *sd = SD_CARD(obj);
>> +
>> +    if (sd->bdrv) {
>> +        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(sd->bdrv));
>> +    } else {
>> +        visit_type_bool(v, &sd->spi, name, errp);
>> +    }
>> +}
>> +
>> +static void sd_initfn(Object *obj)
>> +{
>> +    SDState *sd = SD_CARD(obj);
>> +
>> +    sd->spi = false;
>> +    object_property_add(obj, "spi", "boolean", sd_is_spi, sd_set_spimode,
>> +            NULL, NULL, NULL);
>> +}
>> +
>>  static const TypeInfo sd_type_info = {
>>      .name = TYPE_SD_CARD,
>>      .parent = TYPE_OBJECT,
>>      .instance_size = sizeof(SDState),
>> +    .instance_init = sd_initfn,
>>      .class_init = sd_class_init,
>>      .class_size = sizeof(SDClass)
>>  };
> 
> I suspect this would be much simpler the declarative way qdevs normally
> use.  For an example, check out scsi_hd_properties[] and its use in
> hw/scsi-disk.c.
[snip]

For static properties bool support was missing some time ago...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH V4 11/12] SD card: introduce "spi" property for SD card objects

[Paolo Bonzini]

Il 31/07/2012 14:19, Andreas Färber ha scritto:
>>> >> +    sd->spi = false;
>>> >> +    object_property_add(obj, "spi", "boolean", sd_is_spi, sd_set_spimode,
>>> >> +            NULL, NULL, NULL);
>>> >> +}
>>> >> +
>>> >>  static const TypeInfo sd_type_info = {
>>> >>      .name = TYPE_SD_CARD,
>>> >>      .parent = TYPE_OBJECT,
>>> >>      .instance_size = sizeof(SDState),
>>> >> +    .instance_init = sd_initfn,
>>> >>      .class_init = sd_class_init,
>>> >>      .class_size = sizeof(SDClass)
>>> >>  };
>> > 
>> > I suspect this would be much simpler the declarative way qdevs normally
>> > use.  For an example, check out scsi_hd_properties[] and its use in
>> > hw/scsi-disk.c.
> [snip]
> 
> For static properties bool support was missing some time ago...

There are bitfields, which are really the same thing except they expect
an u32 field.

Paolo

[Qemu-devel] [PATCH V4 12/12] hw/sd.c: introduce SD card "drive" property

[Igor Mitsyanko]

Setting "drive" SD card property ties SD card with BlockDriverState of the
same name. This property can be set dynamically, allowing for SD card hot-insert.

With "drive" property we no longer need SDClass::init method, all work is
done in property setter.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
---
 hw/sd.c |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/sd.h |    7 ++++-
 2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/hw/sd.c b/hw/sd.c
index 611e1f3..fabb046 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -491,15 +491,24 @@ 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.  */
-static void sd_init_card(SDState *sd, BlockDriverState *bs)
+static void sd_init_card(SDState *sd, BlockDriverState *bdrv, Error **errp)
 {
-    sd->buf = qemu_blockalign(bs, 512);
-    sd->enable = true;
-    sd_reset(sd, bs);
-    if (sd->bdrv) {
-        bdrv_attach_dev_nofail(sd->bdrv, sd);
-        bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
+    if (!bdrv) {
+        error_set(errp, QERR_MISSING_PARAMETER, "BlockDriverState");
+        return;
+    }
+
+    if (bdrv_attach_dev(bdrv, sd) < 0) {
+        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bdrv));
+        return;
     }
+
+    bdrv_set_dev_ops(bdrv, &sd_block_ops, sd);
+    sd->buf = qemu_blockalign(bdrv, 512);
+    sd->enable = true;
+    sd_reset(sd, bdrv);
+    qemu_set_irq(sd->inserted_cb, bdrv_is_inserted(sd->bdrv));
+    qemu_set_irq(sd->readonly_cb, sd->wp_switch);
     vmstate_register(NULL, -1, &sd_vmstate, sd);
 }
 
@@ -1756,7 +1765,6 @@ 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;
@@ -1785,6 +1793,54 @@ static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+static char *sd_drive_get(Object *obj, Error **errp)
+{
+    SDState *sd = SD_CARD(obj);
+    char *ret = NULL;
+
+    if (sd->bdrv) {
+        ret = g_strndup(bdrv_get_device_name(sd->bdrv), 32);
+    } else {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, "SD card");
+    }
+
+    return ret;
+}
+
+static void sd_drive_set(Object *obj, const char *value, Error **errp)
+{
+    SDState *sd = SD_CARD(obj);
+    BlockDriverState *new_bdrv = bdrv_find(value);
+
+    if (!new_bdrv) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, value);
+        return;
+    }
+
+    if (sd->bdrv) {
+        if (!strncmp(bdrv_get_device_name(sd->bdrv), value, 32)) {
+            return;
+        }
+
+        if (bdrv_in_use(sd->bdrv)) {
+            error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(sd->bdrv));
+            return;
+        }
+
+        bdrv_close(sd->bdrv);
+        sd->enable = false;
+        bdrv_detach_dev(sd->bdrv, sd);
+        vmstate_unregister(NULL, &sd_vmstate, sd);
+        if (sd->buf) {
+            qemu_vfree(sd->buf);
+            sd->buf = NULL;
+        }
+        sd_reset(sd, NULL);
+    }
+
+    sd_init_card(sd, new_bdrv, errp);
+}
+
 static void sd_initfn(Object *obj)
 {
     SDState *sd = SD_CARD(obj);
@@ -1792,6 +1848,8 @@ static void sd_initfn(Object *obj)
     sd->spi = false;
     object_property_add(obj, "spi", "boolean", sd_is_spi, sd_set_spimode,
             NULL, NULL, NULL);
+    object_property_add_str(OBJECT(sd), "drive", sd_drive_get, sd_drive_set,
+        NULL);
 }
 
 static const TypeInfo sd_type_info = {
diff --git a/hw/sd.h b/hw/sd.h
index c78eaa1..acf5d94 100644
--- a/hw/sd.h
+++ b/hw/sd.h
@@ -32,6 +32,7 @@
 #include "qemu-common.h"
 #include "qemu/object.h"
 #include "qapi/qapi-visit-core.h"
+#include "block.h"
 
 #define OUT_OF_RANGE		(1 << 31)
 #define ADDRESS_ERROR		(1 << 30)
@@ -74,7 +75,6 @@ typedef struct SDState SDState;
 typedef struct SDClass {
     ObjectClass parent_class;
 
-    void (*init)(SDState *sd, BlockDriverState *bdrv);
     int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
     void (*write_data)(SDState *sd, uint8_t value);
     uint8_t (*read_data)(SDState *sd);
@@ -96,8 +96,11 @@ static inline SDState *sd_init(BlockDriverState *bs, bool is_spi)
     SDState *sd = SD_CARD(object_new(TYPE_SD_CARD));
     Error *errp = NULL;
     object_property_set_bool(OBJECT(sd), is_spi, "spi", &errp);
+    if (bs) {
+        object_property_set_str(OBJECT(sd), bdrv_get_device_name(bs),
+                "drive", &errp);
+    }
     assert_no_error(errp);
-    SD_GET_CLASS(sd)->init(sd, bs);
     return sd;
 }
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH V4 00/12] SD save/load support, SD qomification and bug fixes

[Peter Maydell]

On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Igor Mitsyanko (12):
>   hw/sd.c: convert wp_groups in SDState to bitfield
>   hw/sd.c: make sd_wp_addr() accept 64 bit address argument
>   hw/sd.c: introduce wrapper for conversion address to wp group
>   hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase
>   hw/sd.c: convert binary variables to bool
>   hw/sd.c: make sd_dataready() return bool
>   hw/sd.c: make sd_wp_addr() return bool
>   hw/sd.c: add SD card save/load support
>   hw/sd.c: convert SD state to QOM object
>   SD card users: optimize access to SDClass methods
>   SD card: introduce "spi" property for SD card objects
>   hw/sd.c: introduce SD card "drive" property

I'm taking patches 1,2,3,5,6,7 into arm-devs.next because they're
uncontroversial standalone cleanup patches.

-- PMM

Re: [Qemu-devel] [PATCH V4 00/12] SD save/load support, SD qomification and bug fixes

[Igor Mitsyanko]

On 08/10/2012 07:06 PM, Peter Maydell wrote:
> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> Igor Mitsyanko (12):
>>    hw/sd.c: convert wp_groups in SDState to bitfield
>>    hw/sd.c: make sd_wp_addr() accept 64 bit address argument
>>    hw/sd.c: introduce wrapper for conversion address to wp group
>>    hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase
>>    hw/sd.c: convert binary variables to bool
>>    hw/sd.c: make sd_dataready() return bool
>>    hw/sd.c: make sd_wp_addr() return bool
>>    hw/sd.c: add SD card save/load support
>>    hw/sd.c: convert SD state to QOM object
>>    SD card users: optimize access to SDClass methods
>>    SD card: introduce "spi" property for SD card objects
>>    hw/sd.c: introduce SD card "drive" property
> I'm taking patches 1,2,3,5,6,7 into arm-devs.next because they're
> uncontroversial standalone cleanup patches.
>
> -- PMM
>
Ok, nice

Re: [Qemu-devel] [PATCH V4 00/12] SD save/load support, SD qomification and bug fixes

[Peter Maydell]

On 10 August 2012 17:23, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 08/10/2012 07:06 PM, Peter Maydell wrote:
>>
>> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>
>>> Igor Mitsyanko (12):
>>>    hw/sd.c: convert wp_groups in SDState to bitfield
>>>    hw/sd.c: make sd_wp_addr() accept 64 bit address argument
>>>    hw/sd.c: introduce wrapper for conversion address to wp group
>>>    hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase
>>>    hw/sd.c: convert binary variables to bool
>>>    hw/sd.c: make sd_dataready() return bool
>>>    hw/sd.c: make sd_wp_addr() return bool
>>>    hw/sd.c: add SD card save/load support
>>>    hw/sd.c: convert SD state to QOM object
>>>    SD card users: optimize access to SDClass methods
>>>    SD card: introduce "spi" property for SD card objects
>>>    hw/sd.c: introduce SD card "drive" property
>>
>> I'm taking patches 1,2,3,5,6,7 into arm-devs.next because they're
>> uncontroversial standalone cleanup patches.

> Ok, nice

Were you planning to try to do another version of this before the
code freeze (coming up end of the month)?

-- PMM

Re: [Qemu-devel] [PATCH V4 00/12] SD save/load support, SD qomification and bug fixes

[Igor Mitsyanko]

On 10/25/2012 07:47 PM, Peter Maydell wrote:
> On 10 August 2012 17:23, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>> On 08/10/2012 07:06 PM, Peter Maydell wrote:
>>> On 27 July 2012 20:29, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
>>>> Igor Mitsyanko (12):
>>>>     hw/sd.c: convert wp_groups in SDState to bitfield
>>>>     hw/sd.c: make sd_wp_addr() accept 64 bit address argument
>>>>     hw/sd.c: introduce wrapper for conversion address to wp group
>>>>     hw/sd.c: favour SD card type (SDSC or SDHC) when performing erase
>>>>     hw/sd.c: convert binary variables to bool
>>>>     hw/sd.c: make sd_dataready() return bool
>>>>     hw/sd.c: make sd_wp_addr() return bool
>>>>     hw/sd.c: add SD card save/load support
>>>>     hw/sd.c: convert SD state to QOM object
>>>>     SD card users: optimize access to SDClass methods
>>>>     SD card: introduce "spi" property for SD card objects
>>>>     hw/sd.c: introduce SD card "drive" property
>>> I'm taking patches 1,2,3,5,6,7 into arm-devs.next because they're
>>> uncontroversial standalone cleanup patches.
>> Ok, nice
> Were you planning to try to do another version of this before the
> code freeze (coming up end of the month)?
>
> -- PMM
>
I can do it on these weekends (saturday), I've been away for a long time 
until recently and planned/did nothing in the mean time.

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com