Re: [Qemu-devel] [PATCH v5 03/15] ssi: Implemented CS behaviour

[Peter Crosthwaite]

On Mon, Aug 6, 2012 at 7:25 PM, Peter Maydell <address@hidden> wrote:
> On 6 August 2012 03:16, Peter A. G. Crosthwaite
> <address@hidden> wrote:
>> 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 <address@hidden>
>> ---
>>  hw/ssd0323.c |    6 ++++++
>>  hw/ssi-sd.c  |    6 ++++++
>>  hw/ssi.c     |   39 ++++++++++++++++++++++++++++++++++++++-
>>  hw/ssi.h     |   27 +++++++++++++++++++++++++++
>>  4 files changed, 77 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/ssd0323.c b/hw/ssd0323.c
>> index b0b2e94..db16d20 100644
>> --- a/hw/ssd0323.c
>> +++ b/hw/ssd0323.c
>> @@ -277,6 +277,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;
>>
>> @@ -294,10 +295,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 ? 1 : 0);
>
> You don't need this ?: -- the standard bool-to-integer casting
> rules will do it for you. Also, stray blank line, and you're
> changing save/load format so you need to update a version number
> somewhere.
>

Ok, any quick guidelines for where to find information on how to do this?

>>  }
>>
>>  static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
>>  {
>> +    SSISlave *ss = SSI_SLAVE(opaque);
>>      ssd0323_state *s = (ssd0323_state *)opaque;
>>      int i;
>>
>> @@ -319,6 +323,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);
>
> !! unnecessary here.
>
> Same comments as here and the one above apply to the other devices
> below.
>
>> +
>>      return 0;
>>  }
>>
>> diff --git a/hw/ssi-sd.c b/hw/ssi-sd.c
>> index b519bdb..6fd9ab9 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 ? 1 : 0);
>>  }
>>
>>  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 2db88fc..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,7 +110,7 @@ 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;
>> @@ -86,6 +122,7 @@ const VMStateDescription vmstate_ssi_slave = {
>>      .minimum_version_id = 1,
>>      .minimum_version_id_old = 1,
>>      .fields      = (VMStateField[]) {
>> +        VMSTATE_BOOL(cs, SSISlave),
>
> New field -> must bump version. You could probably avoid this by
> rearranging things so this field is in the vmstate from the start
> rather than added in a subsequent patch.
>

So is this a bisect-ability issue between this and the previous patch?
If so Ill just fold the two together, they are related enough to do
so. Better to not change functionality for the sake of revision
control.

>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
>> diff --git a/hw/ssi.h b/hw/ssi.h
>> index 975f9fb..5b69a3b 100644
>> --- a/hw/ssi.h
>> +++ b/hw/ssi.h
>> @@ -23,16 +23,43 @@ 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 assoicated with the cs line (beyond
>
> "associated"
>
>> +     * 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 behvaiour override this to take control
>
> "behaviour" (and below)
>
>> +     * of the CS behvaiour at the device level. transfer, set_cs, and
>> +     * cs_polarity are unused if this is overwritten. Transfer_raw, will
>
> spurious comma
>
>> +     * always be called for the device for every txrx access the to parent 
>> bus
>
> "to the"
>
>> +     */
>> +    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)
>> --
>> 1.7.0.4
>>
>
>
> -- PMM

Regards,
Peter