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

[Peter Maydell]

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.

>  }
>
>  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.

>          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