diff mbox

[v7,02/13] ssi: Implemented CS behaviour

Message ID 08f375950ce1bb8adafa564cc5bc60489692d828.1348104012.git.peter.crosthwaite@petalogix.com
State New
Headers show

Commit Message

Peter A. G. Crosthwaite Sept. 24, 2012, 9:18 a.m. UTC 
Added default CS behaviour for SSI slaves. SSI devices can set a property
to enable CS behaviour which will create a GPIO on the device which is the
CS. Tristating of the bus on SSI transfers is implemented.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
Changed since v5:
Addressed PMM review.
Collapsed into one patch for bisectability (this used to be two patches)

 hw/ads7846.c   |    7 ++++---
 hw/max111x.c   |    7 ++++---
 hw/spitz.c     |    8 +++++---
 hw/ssd0323.c   |    6 ++++++
 hw/ssi-sd.c    |    6 ++++++
 hw/ssi.c       |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ssi.h       |   37 +++++++++++++++++++++++++++++++++++++
 hw/stellaris.c |    7 ++++---
 hw/z2.c        |    7 ++++---
 9 files changed, 118 insertions(+), 16 deletions(-)

Comments

Peter Maydell Sept. 24, 2012, 9:29 a.m. UTC | #1 
On 24 September 2012 10:18, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> @@ -296,10 +297,13 @@ static void ssd0323_save(QEMUFile *f, void *opaque)
>      qemu_put_be32(f, s->remap);
>      qemu_put_be32(f, s->mode);
>      qemu_put_buffer(f, s->framebuffer, sizeof(s->framebuffer));
> +
> +    qemu_put_be32(f, ss->cs);
>  }
>
>  static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>  {
> +    SSISlave *ss = SSI_SLAVE(opaque);
>      ssd0323_state *s = (ssd0323_state *)opaque;
>      int i;
>
> @@ -321,6 +325,8 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>      s->mode = qemu_get_be32(f);
>      qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>
> +    ss->cs = qemu_get_be32(f);
> +
>      return 0;
>  }

This looks odd. The cs field isn't part of the ssd0323_state,
so it shouldn't be ssd0303_save/load's job to save and restore
it. Contrast the way the vmstate-based devices don't directly
save/restore cs but defer to VMSTATE_SSI_SLAVE.

-- PMM
Peter A. G. Crosthwaite Sept. 24, 2012, 10:19 a.m. UTC | #2 
On Mon, Sep 24, 2012 at 7:29 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 September 2012 10:18, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> @@ -296,10 +297,13 @@ static void ssd0323_save(QEMUFile *f, void *opaque)
>>      qemu_put_be32(f, s->remap);
>>      qemu_put_be32(f, s->mode);
>>      qemu_put_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>> +
>> +    qemu_put_be32(f, ss->cs);
>>  }
>>
>>  static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>>  {
>> +    SSISlave *ss = SSI_SLAVE(opaque);
>>      ssd0323_state *s = (ssd0323_state *)opaque;
>>      int i;
>>
>> @@ -321,6 +325,8 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>>      s->mode = qemu_get_be32(f);
>>      qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>>
>> +    ss->cs = qemu_get_be32(f);
>> +
>>      return 0;
>>  }
>
> This looks odd. The cs field isn't part of the ssd0323_state,
> so it shouldn't be ssd0303_save/load's job to save and restore
> it. Contrast the way the vmstate-based devices don't directly
> save/restore cs but defer to VMSTATE_SSI_SLAVE.
>

I took a "when in rome" philosophy here, and did it the way the device
model already does VMSD. The rest of the device state is encapsulated
using this mechanism. Im guessing this is an old way of doing VMSD
with the whole register_savevm() thing. Is it possible to mix and
match this old file-style VMSD with the newer struct approach? I cant
see an easy way without completely rewriting the VMSD implementation
of this device model.

Regards,
Peter

> -- PMM
Peter A. G. Crosthwaite Sept. 24, 2012, 10:56 a.m. UTC | #3 
On Mon, Sep 24, 2012 at 8:19 PM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Mon, Sep 24, 2012 at 7:29 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 September 2012 10:18, Peter A. G. Crosthwaite
>> <peter.crosthwaite@petalogix.com> wrote:
>>> @@ -296,10 +297,13 @@ static void ssd0323_save(QEMUFile *f, void *opaque)
>>>      qemu_put_be32(f, s->remap);
>>>      qemu_put_be32(f, s->mode);
>>>      qemu_put_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>>> +
>>> +    qemu_put_be32(f, ss->cs);
>>>  }
>>>
>>>  static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>>>  {
>>> +    SSISlave *ss = SSI_SLAVE(opaque);
>>>      ssd0323_state *s = (ssd0323_state *)opaque;
>>>      int i;
>>>
>>> @@ -321,6 +325,8 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>>>      s->mode = qemu_get_be32(f);
>>>      qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer));
>>>
>>> +    ss->cs = qemu_get_be32(f);
>>> +
>>>      return 0;
>>>  }
>>
>> This looks odd. The cs field isn't part of the ssd0323_state,
>> so it shouldn't be ssd0303_save/load's job to save and restore
>> it. Contrast the way the vmstate-based devices don't directly
>> save/restore cs but defer to VMSTATE_SSI_SLAVE.
>>
>
> I took a "when in rome" philosophy here, and did it the way the device
> model already does VMSD. The rest of the device state is encapsulated
> using this mechanism. Im guessing this is an old way of doing VMSD
> with the whole register_savevm() thing. Is it possible to mix and
> match this old file-style VMSD with the newer struct approach? I cant
> see an easy way without completely rewriting the VMSD implementation
> of this device model.
>

Hi Juan,

We have another curly VMSD question on this series. The hw/ssd0323.c
uses the old school register_savevm() scheme for VMSD. With this
series, this device now inherits from another device (TYPE_SSI_SLAVE)
which has proper VMSD support. The question is can, we mix and match
the old and new VMSD to avoid having to rewrite this device models
VMSD? Can we leave this device model vmsd as is and add the vm_state
with the single SSI_SLAVE field? This vmsd would then save/load the
inherited state and co-exist with the register_savevm() implementation
for the non-inherited state.

Regards,
Peter

> Regards,
> Peter
>
>> -- PMM
diff mbox

Patch

diff --git a/hw/ads7846.c b/hw/ads7846.c
index 41c7f10..2ea9e55 100644
--- a/hw/ads7846.c
+++ b/hw/ads7846.c
@@ -119,11 +119,12 @@  static int ads7856_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_ads7846 = {
     .name = "ads7846",
-    .version_id = 0,
-    .minimum_version_id = 0,
-    .minimum_version_id_old = 0,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
     .post_load = ads7856_post_load,
     .fields      = (VMStateField[]) {
+        VMSTATE_SSI_SLAVE(ssidev, ADS7846State),
         VMSTATE_INT32_ARRAY(input, ADS7846State, 8),
         VMSTATE_INT32(noise, ADS7846State),
         VMSTATE_INT32(cycle, ADS7846State),
diff --git a/hw/max111x.c b/hw/max111x.c
index 706d89f..67640f1 100644
--- a/hw/max111x.c
+++ b/hw/max111x.c
@@ -99,10 +99,11 @@  static uint32_t max111x_transfer(SSISlave *dev, uint32_t value)
 
 static const VMStateDescription vmstate_max111x = {
     .name = "max111x",
-    .version_id = 0,
-    .minimum_version_id = 0,
-    .minimum_version_id_old = 0,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
     .fields      = (VMStateField[]) {
+        VMSTATE_SSI_SLAVE(ssidev, MAX111xState),
         VMSTATE_UINT8(tb1, MAX111xState),
         VMSTATE_UINT8(rb2, MAX111xState),
         VMSTATE_UINT8(rb3, MAX111xState),
diff --git a/hw/spitz.c b/hw/spitz.c
index 20e7835..24346dc 100644
--- a/hw/spitz.c
+++ b/hw/spitz.c
@@ -1083,10 +1083,11 @@  static TypeInfo spitz_keyboard_info = {
 
 static const VMStateDescription vmstate_corgi_ssp_regs = {
     .name = "corgi-ssp",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields = (VMStateField []) {
+        VMSTATE_SSI_SLAVE(ssidev, CorgiSSPState),
         VMSTATE_UINT32_ARRAY(enable, CorgiSSPState, 3),
         VMSTATE_END_OF_LIST(),
     }
@@ -1115,6 +1116,7 @@  static const VMStateDescription vmstate_spitz_lcdtg_regs = {
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields = (VMStateField []) {
+        VMSTATE_SSI_SLAVE(ssidev, SpitzLCDTG),
         VMSTATE_UINT32(bl_intensity, SpitzLCDTG),
         VMSTATE_UINT32(bl_power, SpitzLCDTG),
         VMSTATE_END_OF_LIST(),
diff --git a/hw/ssd0323.c b/hw/ssd0323.c
index b101c51..5d05a35 100644
--- a/hw/ssd0323.c
+++ b/hw/ssd0323.c
@@ -279,6 +279,7 @@  static void ssd0323_cd(void *opaque, int n, int level)
 
 static void ssd0323_save(QEMUFile *f, void *opaque)
 {
+    SSISlave *ss = SSI_SLAVE(opaque);
     ssd0323_state *s = (ssd0323_state *)opaque;
     int i;
 
@@ -296,10 +297,13 @@  static void ssd0323_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->remap);
     qemu_put_be32(f, s->mode);
     qemu_put_buffer(f, s->framebuffer, sizeof(s->framebuffer));
+
+    qemu_put_be32(f, ss->cs);
 }
 
 static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
 {
+    SSISlave *ss = SSI_SLAVE(opaque);
     ssd0323_state *s = (ssd0323_state *)opaque;
     int i;
 
@@ -321,6 +325,8 @@  static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
     s->mode = qemu_get_be32(f);
     qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer));
 
+    ss->cs = qemu_get_be32(f);
+
     return 0;
 }
 
diff --git a/hw/ssi-sd.c b/hw/ssi-sd.c
index b519bdb..cbbc645 100644
--- a/hw/ssi-sd.c
+++ b/hw/ssi-sd.c
@@ -197,6 +197,7 @@  static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
 
 static void ssi_sd_save(QEMUFile *f, void *opaque)
 {
+    SSISlave *ss = SSI_SLAVE(opaque);
     ssi_sd_state *s = (ssi_sd_state *)opaque;
     int i;
 
@@ -209,10 +210,13 @@  static void ssi_sd_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->arglen);
     qemu_put_be32(f, s->response_pos);
     qemu_put_be32(f, s->stopping);
+
+    qemu_put_be32(f, ss->cs);
 }
 
 static int ssi_sd_load(QEMUFile *f, void *opaque, int version_id)
 {
+    SSISlave *ss = SSI_SLAVE(opaque);
     ssi_sd_state *s = (ssi_sd_state *)opaque;
     int i;
 
@@ -229,6 +233,8 @@  static int ssi_sd_load(QEMUFile *f, void *opaque, int version_id)
     s->response_pos = qemu_get_be32(f);
     s->stopping = qemu_get_be32(f);
 
+    ss->cs = qemu_get_be32(f);
+
     return 0;
 }
 
diff --git a/hw/ssi.c b/hw/ssi.c
index 35d0a04..2e4f2fe 100644
--- a/hw/ssi.c
+++ b/hw/ssi.c
@@ -27,19 +27,55 @@  static const TypeInfo ssi_bus_info = {
     .instance_size = sizeof(SSIBus),
 };
 
+static void ssi_cs_default(void *opaque, int n, int level)
+{
+    SSISlave *s = SSI_SLAVE(opaque);
+    bool cs = !!level;
+    assert(n == 0);
+    if (s->cs != cs) {
+        SSISlaveClass *ssc = SSI_SLAVE_GET_CLASS(s);
+        if (ssc->set_cs) {
+            ssc->set_cs(s, cs);
+        }
+    }
+    s->cs = cs;
+}
+
+static uint32_t ssi_transfer_raw_default(SSISlave *dev, uint32_t val)
+{
+    SSISlaveClass *ssc = SSI_SLAVE_GET_CLASS(dev);
+
+    if ((dev->cs && ssc->cs_polarity == SSI_CS_HIGH) ||
+            (!dev->cs && ssc->cs_polarity == SSI_CS_LOW) ||
+            ssc->cs_polarity == SSI_CS_NONE) {
+        return ssc->transfer(dev, val);
+    }
+    return 0;
+}
+
 static int ssi_slave_init(DeviceState *dev)
 {
     SSISlave *s = SSI_SLAVE(dev);
     SSISlaveClass *ssc = SSI_SLAVE_GET_CLASS(s);
 
+    if (ssc->transfer_raw == ssi_transfer_raw_default &&
+            ssc->cs_polarity != SSI_CS_NONE) {
+        qdev_init_gpio_in(&s->qdev, ssi_cs_default, 1);
+    }
+
     return ssc->init(s);
 }
 
 static void ssi_slave_class_init(ObjectClass *klass, void *data)
 {
+    SSISlaveClass *ssc = SSI_SLAVE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
+
     dc->init = ssi_slave_init;
     dc->bus_type = TYPE_SSI_BUS;
+    if (!ssc->transfer_raw) {
+        ssc->transfer_raw = ssi_transfer_raw_default;
+    }
 }
 
 static TypeInfo ssi_slave_info = {
@@ -74,12 +110,23 @@  uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
     QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
         SSISlave *slave = SSI_SLAVE(kid->child);
         ssc = SSI_SLAVE_GET_CLASS(slave);
-        r |= ssc->transfer(slave, val);
+        r |= ssc->transfer_raw(slave, val);
     }
 
     return r;
 }
 
+const VMStateDescription vmstate_ssi_slave = {
+    .name = "SSISlave",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_BOOL(cs, SSISlave),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void ssi_slave_register_types(void)
 {
     type_register_static(&ssi_bus_info);
diff --git a/hw/ssi.h b/hw/ssi.h
index 06657d7..65b159d 100644
--- a/hw/ssi.h
+++ b/hw/ssi.h
@@ -23,21 +23,58 @@  typedef struct SSISlave SSISlave;
 #define SSI_SLAVE_GET_CLASS(obj) \
      OBJECT_GET_CLASS(SSISlaveClass, (obj), TYPE_SSI_SLAVE)
 
+typedef enum {
+    SSI_CS_NONE = 0,
+    SSI_CS_LOW,
+    SSI_CS_HIGH,
+} SSICSMode;
+
 /* Slave devices.  */
 typedef struct SSISlaveClass {
     DeviceClass parent_class;
 
     int (*init)(SSISlave *dev);
+
+    /* if you have standard or no CS behaviour, just override transfer.
+     * This is called when the device cs is active (true by default).
+     */
     uint32_t (*transfer)(SSISlave *dev, uint32_t val);
+    /* called when the CS line changes. Optional, devices only need to implement
+     * this if they have side effects associated with the cs line (beyond
+     * tristating the txrx lines).
+     */
+    int (*set_cs)(SSISlave *dev, bool select);
+    /* define whether or not CS exists and is active low/high */
+    SSICSMode cs_polarity;
+
+    /* if you have non-standard CS behaviour override this to take control
+     * of the CS behaviour at the device level. transfer, set_cs, and
+     * cs_polarity are unused if this is overwritten. Transfer_raw will
+     * always be called for the device for every txrx access to the parent bus
+     */
+    uint32_t (*transfer_raw)(SSISlave *dev, uint32_t val);
 } SSISlaveClass;
 
 struct SSISlave {
     DeviceState qdev;
+
+    /* Chip select state */
+    bool cs;
 };
 
 #define SSI_SLAVE_FROM_QDEV(dev) DO_UPCAST(SSISlave, qdev, dev)
 #define FROM_SSI_SLAVE(type, dev) DO_UPCAST(type, ssidev, dev)
 
+extern const VMStateDescription vmstate_ssi_slave;
+
+#define VMSTATE_SSI_SLAVE(_field, _state) {                          \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(SSISlave),                                  \
+    .vmsd       = &vmstate_ssi_slave,                                \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, SSISlave),    \
+}
+
 DeviceState *ssi_create_slave(SSIBus *bus, const char *name);
 
 /* Master interface.  */
diff --git a/hw/stellaris.c b/hw/stellaris.c
index 562fbbf..01050d1 100644
--- a/hw/stellaris.c
+++ b/hw/stellaris.c
@@ -1184,10 +1184,11 @@  static uint32_t stellaris_ssi_bus_transfer(SSISlave *dev, uint32_t val)
 
 static const VMStateDescription vmstate_stellaris_ssi_bus = {
     .name = "stellaris_ssi_bus",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields      = (VMStateField[]) {
+        VMSTATE_SSI_SLAVE(ssidev, stellaris_ssi_bus_state),
         VMSTATE_INT32(current_dev, stellaris_ssi_bus_state),
         VMSTATE_END_OF_LIST()
     }
diff --git a/hw/z2.c b/hw/z2.c
index 289cee9..076fad2 100644
--- a/hw/z2.c
+++ b/hw/z2.c
@@ -161,10 +161,11 @@  static int zipit_lcd_init(SSISlave *dev)
 
 static VMStateDescription vmstate_zipit_lcd_state = {
     .name = "zipit-lcd",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields = (VMStateField[]) {
+        VMSTATE_SSI_SLAVE(ssidev, ZipitLCD),
         VMSTATE_INT32(selected, ZipitLCD),
         VMSTATE_INT32(enabled, ZipitLCD),
         VMSTATE_BUFFER(buf, ZipitLCD),