[RFC,v1,1/4] SPI: initial support

defined spi bus and spi slave QOM interfaces. Inspired by and loosley based on
existing I2C framework.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 Makefile.target |    1 +
 hw/spi.c        |  175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/spi.h        |   86 +++++++++++++++++++++++++++
 3 files changed, 262 insertions(+), 0 deletions(-)
 create mode 100644 hw/spi.c
 create mode 100644 hw/spi.h

On 30 March 2012 07:37, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> defined spi bus and spi slave QOM interfaces. Inspired by and loosley based on
> existing I2C framework.

There's also SPI support in qemu-linaro thanks to the folks at Nokia:
http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=blob;f=hw/spi.h

I guess we need to look at both and figure out which is the better
implementation...

-- PMM

Hi Peter,

One major flaw in that interface (which is similar to another that we
originally used) is it doesnt have an API for wiggling the CS lines.
Some spi devices (e.g. the m25p80 in this series) have side effects
from cs strobing whether or not a txrx occurs during cs assertion,
which is not possible in that api. Thats the main difference between
the two implementations.

Regards,
Peter


On Fri, Mar 30, 2012 at 5:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 March 2012 07:37, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> defined spi bus and spi slave QOM interfaces. Inspired by and loosley based on
>> existing I2C framework.
>
> There's also SPI support in qemu-linaro thanks to the folks at Nokia:
> http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=blob;f=hw/spi.h
>
> I guess we need to look at both and figure out which is the better
> implementation...
>
> -- PMM

On 30 March 2012 07:37, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> defined spi bus and spi slave QOM interfaces. Inspired by and loosley based on

"Define"; "loosely".

> existing I2C framework.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  Makefile.target |    1 +
>  hw/spi.c        |  175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/spi.h        |   86 +++++++++++++++++++++++++++
>  3 files changed, 262 insertions(+), 0 deletions(-)
>  create mode 100644 hw/spi.c
>  create mode 100644 hw/spi.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 44b2e83..8fd3718 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -320,6 +320,7 @@ obj-mips-$(CONFIG_FULONG) += bonito.o vt82c686.o mips_fulong2e.o
>  obj-microblaze-y = petalogix_s3adsp1800_mmu.o
>  obj-microblaze-y += petalogix_ml605_mmu.o
>  obj-microblaze-y += microblaze_boot.o
> +obj-microblaze-y += spi.o

This should be in common-obj-y with i2c.o, I think -- microblaze isn't
the only target with SPI (eg hw/omap_spi.c).

>  obj-microblaze-y += microblaze_pic_cpu.o
>  obj-microblaze-y += xilinx_intc.o
> diff --git a/hw/spi.c b/hw/spi.c
> new file mode 100644
> index 0000000..3afef07
> --- /dev/null
> +++ b/hw/spi.c
> @@ -0,0 +1,175 @@
> +/*
> + * QEMU SPI bus interface.
> + *
> + * Copyright (C) 2012 Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> + * Copyright (C) 2012 PetaLogix
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "spi.h"
> +
> +static struct BusInfo spi_bus_info = {
> +    .name = "SPI",
> +    .size = sizeof(spi_bus)
> +};
> +
> +static const VMStateDescription vmstate_spi_bus = {
> +    .name = "spi_bus",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8(cur_slave, spi_bus),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +spi_bus *spi_init_bus(DeviceState *parent, int num_slaves, const char *name)
> +{
> +    spi_bus *bus;
> +
> +    bus = FROM_QBUS(spi_bus, qbus_create(&spi_bus_info, parent, name));
> +    if (num_slaves >= SPI_BUS_NO_CS) {
> +        hw_error("too many slaves on spi bus: %d\n", num_slaves);
> +    }
> +    bus->num_slaves = num_slaves;
> +    bus->slaves = g_malloc0(sizeof(*bus->slaves) * num_slaves);
> +    vmstate_register(NULL, -1, &vmstate_spi_bus, bus);
> +    return bus;
> +}
> +
> +int spi_attach_slave(spi_bus *bus, SPISlave *slave, int cs)
> +{
> +    if (bus->slaves[cs]) {
> +        return 1;
> +    }
> +    bus->slaves[cs] = slave;
> +    return 0;
> +}
> +
> +int spi_set_cs(spi_bus *bus, int cs)
> +{
> +    SPISlave *dev;
> +    SPISlaveClass *klass;
> +
> +    if (bus->cur_slave == cs) {
> +        return 0;
> +    }
> +
> +    if (bus->cur_slave != SPI_BUS_NO_CS) {
> +        dev = bus->slaves[bus->cur_slave];
> +        dev->cs = 0;
> +        klass = SPI_SLAVE_GET_CLASS(dev);
> +        klass->cs(dev, 0);
> +    }
> +
> +    if (cs >= bus->num_slaves && cs != SPI_BUS_NO_CS) {
> +        hw_error("attempted to assert non existent spi CS line: %d\n", cs);
> +    }
> +
> +    bus->cur_slave = (uint8_t)cs;
> +
> +    if (cs != SPI_BUS_NO_CS) {
> +        dev = bus->slaves[cs];
> +        dev->cs = 1;
> +        klass = SPI_SLAVE_GET_CLASS(dev);
> +        klass->cs(dev, 1);
> +    }
> +    return 0;
> +};
> +
> +int spi_get_cs(spi_bus *bus)
> +{
> +    return bus->cur_slave;
> +}
> +
> +SpiSlaveState spi_get_state(spi_bus *bus)
> +{
> +    SPISlave *dev;
> +    SPISlaveClass *klass;
> +
> +    if (bus->cur_slave == SPI_BUS_NO_CS) {
> +        return SPI_NO_CS;
> +    }
> +    dev = bus->slaves[bus->cur_slave];
> +    klass = SPI_SLAVE_GET_CLASS(dev);
> +
> +    return klass->get_state(dev);
> +}
> +
> +SpiSlaveState spi_send(spi_bus *bus, uint32_t data, int len)
> +{
> +    SPISlave *dev;
> +    SPISlaveClass *klass;
> +
> +    if (bus->cur_slave == SPI_BUS_NO_CS) {
> +        return SPI_NO_CS;
> +    }
> +    dev = bus->slaves[bus->cur_slave];
> +    klass = SPI_SLAVE_GET_CLASS(dev);
> +
> +    return klass->send(dev, data, len);
> +}
> +
> +SpiSlaveState spi_recv(spi_bus *bus, uint32_t *data)
> +{
> +    SPISlave *dev;
> +    SPISlaveClass *klass;
> +
> +    if (bus->cur_slave == SPI_BUS_NO_CS) {
> +        return SPI_NO_CS;
> +    }
> +    dev = bus->slaves[bus->cur_slave];
> +    klass = SPI_SLAVE_GET_CLASS(dev);
> +
> +    return klass->recv(dev, data);
> +}
> +
> +const VMStateDescription vmstate_spi_slave = {
> +    .name = "SPISlave",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8(cs, SPISlave),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static int spi_slave_qdev_init(DeviceState *dev)
> +{
> +    SPISlave *s = SPI_SLAVE_FROM_QDEV(dev);
> +    SPISlaveClass *sc = SPI_SLAVE_GET_CLASS(s);
> +
> +    return sc->init(s);
> +}
> +
> +static void spi_slave_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *k = DEVICE_CLASS(klass);
> +    k->init = spi_slave_qdev_init;
> +    k->bus_info = &spi_bus_info;
> +}
> +
> +static TypeInfo spi_slave_type_info = {
> +    .name = TYPE_SPI_SLAVE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(SPISlave),
> +    .abstract = true,
> +    .class_size = sizeof(SPISlaveClass),
> +    .class_init = spi_slave_class_init,
> +};
> +
> +static void spi_slave_register_types(void)
> +{
> +    type_register_static(&spi_slave_type_info);
> +}
> +
> +type_init(spi_slave_register_types)
> diff --git a/hw/spi.h b/hw/spi.h
> new file mode 100644
> index 0000000..668e9b0
> --- /dev/null
> +++ b/hw/spi.h
> @@ -0,0 +1,86 @@
> +#ifndef QEMU_SPI_H
> +#define QEMU_SPI_H
> +
> +#include "qdev.h"
> +
> +/* pass to spi_set_cs to deslect all devices on bus */

"deselect"

> +
> +#define SPI_BUS_NO_CS 0xFF
> +
> +/* state of a SPI device,
> + * SPI_NO_CS -> the CS pin in de-asserted -> device is tristated

"is"

> + * SPI_IDLE -> CS is asserted and device ready to recv
> + * SPI_DATA_PENDING -> CS is asserted and the device has pushed data to master
> + */
> +
> +typedef enum {
> +    SPI_NO_CS,
> +    SPI_IDLE,
> +    SPI_DATA_PENDING
> +} SpiSlaveState;

Why not SPISlaveState ?

> +
> +typedef struct SPISlave {
> +    DeviceState qdev;
> +    uint8_t cs;
> +} SPISlave;
> +
> +#define TYPE_SPI_SLAVE "spi-slave"
> +#define SPI_SLAVE(obj) \
> +     OBJECT_CHECK(SPISlave, (obj), TYPE_SPI_SLAVE)
> +#define SPI_SLAVE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(SPISlaveClass, (klass), TYPE_SPI_SLAVE)
> +#define SPI_SLAVE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(SPISlaveClass, (obj), TYPE_SPI_SLAVE)
> +
> +typedef struct SPISlaveClass {
> +    DeviceClass parent_class;
> +
> +    /* Callbacks provided by the device.  */
> +    int (*init)(SPISlave *s);
> +
> +    /* change the cs pin state */
> +    void (*cs)(SPISlave *s, uint8_t select);
> +
> +    /* Master to slave.  */
> +    SpiSlaveState (*send)(SPISlave *s, uint32_t data, int len);
> +
> +    /* Slave to master.  */
> +    SpiSlaveState (*recv)(SPISlave *s, uint32_t *data);
> +
> +    /* poll the spi device state */
> +    SpiSlaveState (*get_state)(SPISlave *s);
> +} SPISlaveClass;
> +
> +#define SPI_SLAVE_FROM_QDEV(dev) DO_UPCAST(SPISlave, qdev, dev)
> +#define FROM_SPI_SLAVE(type, dev) DO_UPCAST(type, spi, dev)
> +
> +extern const VMStateDescription vmstate_spi_slave;
> +
> +#define VMSTATE_SPI_SLAVE(_field, _state) {                          \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(SPISlave),                                  \
> +    .vmsd       = &vmstate_spi_slave,                                \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, SPISlave),    \
> +}
> +
> +typedef struct spi_bus {
> +    BusState qbus;
> +    SPISlave **slaves;
> +    uint8_t num_slaves;
> +    uint8_t cur_slave;
> +} spi_bus;

CODING_STYLE demands camelcase for type names, so SPIBus.

> +
> +/* create a new spi bus */
> +spi_bus *spi_init_bus(DeviceState *parent, int num_slaves, const char *name);
> +int spi_attach_slave(spi_bus *bus, SPISlave *s, int cs);
> +
> +/* change the chip select. Return 1 on failure. */
> +int spi_set_cs(spi_bus *bus, int cs);
> +int spi_get_cs(spi_bus *bus);
> +SpiSlaveState spi_get_state(spi_bus *bus);
> +
> +SpiSlaveState spi_send(spi_bus *bus, uint32_t data, int len);
> +SpiSlaveState spi_recv(spi_bus *bus, uint32_t *data);

I'm no SPI expert, but a bit of googling suggests that it's
a synchronous duplex bus, so you always send a byte of data
to the slave and get one back in return (even if for some slaves
it might be random garbage). The current OMAP SPI devices in QEMU
master have an API that reflects this: eg tsc210x_txrx() which
takes an input value and returns an output value. This API
seems to have separate send and recv methods -- can you explain
how this works?

(Incidentally if we're going to qdevify the SPI interface we
should also convert the existing omap SPI devices...)

thanks
-- PMM

>> +
>> +/* create a new spi bus */
>> +spi_bus *spi_init_bus(DeviceState *parent, int num_slaves, const char *name);
>> +int spi_attach_slave(spi_bus *bus, SPISlave *s, int cs);
>> +
>> +/* change the chip select. Return 1 on failure. */
>> +int spi_set_cs(spi_bus *bus, int cs);
>> +int spi_get_cs(spi_bus *bus);
>> +SpiSlaveState spi_get_state(spi_bus *bus);
>> +
>> +SpiSlaveState spi_send(spi_bus *bus, uint32_t data, int len);
>> +SpiSlaveState spi_recv(spi_bus *bus, uint32_t *data);
>
> I'm no SPI expert, but a bit of googling suggests that it's
> a synchronous duplex bus, so you always send a byte of data
> to the slave and get one back in return (even if for some slaves
> it might be random garbage).

Hi Peter,

Thats not correct, SPI txs can trigger rxs of arbitrary length. Heres
the data sheet for the m25p80 spi flash device:
www.datasheetcatalog.org/datasheet/stmicroelectronics/8495.pdf.
Figure 12, page 15 is a good example of this, the master transmits 4
bytes back to back for a opcode and an addresses, and the slave bursts
the entire playload back. To hack around this, Edgar had to put escape
sequences in that txrx api to flag all the txs and rxs for devices and
controllers respecitvely, ive attached a file (xilinx_spi.h) which is
Eds original hack of the txrx interface that im trying to fix here by
changing the API.

This api allows controllers to manage that without dummy txs or invalid rxs.

The current OMAP SPI devices in QEMU
> master have an API that reflects this: eg tsc210x_txrx() which
> takes an input value and returns an output value. This API
> seems to have separate send and recv methods -- can you explain
> how this works?
>

Each send or receive command will return a enum SPISlaveState. If
SPI_DATA_PENDING the master can receive a byte back from the slave
with recv. If that recv returns SPI_DATA_PENDING then the master
should recieve again i.e. recv should be looped. Heres how its handled
in the xilinx_spi controller:

    uint32_t value;
    SpiSlaveState st;

    while (!txfifo_empty(s)) {
        value = txfifo_get(s);
        DB_PRINT("data transfer:%x\n", value);
        st = spi_send(s->spi, value, 8);
        while (st == SPI_DATA_PENDING) {
            uint32_t d;
            st = spi_recv(s->spi, &d);
            rxfifo_put(s, d);
        }
    }

The inner loop is basically a poll, waiting for the recv to return !=
SPI_DATA_PENDING.

> (Incidentally if we're going to qdevify the SPI interface we
> should also convert the existing omap SPI devices...)

Yes I havent looked into the details, but shouldnt be too much work. I
can provide the diff for the m25p80 to change it over from txrx style
to this, to give an idea of the change pattern. Little bit of this
that and the other in the machine models too.

If we really want to minimise work, the old txrx function can be added
(to spi.c) as something like:

uint32_t spi_txrx(spi_bus *bus, uint32_t data) {
      uint32_t ret;

      assert(spi_send(bus, data) == SPI_DATA_PENDING);
      assert(spi_recv(bus, &ret) == SPI_DATA_IDLE);
      return ret;
}

>
> thanks
> -- PMM

Regards,
Peter

On Tue, Apr 3, 2012 at 3:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 March 2012 07:37, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> defined spi bus and spi slave QOM interfaces. Inspired by and loosley based on
>
> "Define"; "loosely".
>

ack

>> existing I2C framework.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>>  Makefile.target |    1 +
>>  hw/spi.c        |  175 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/spi.h        |   86 +++++++++++++++++++++++++++
>>  3 files changed, 262 insertions(+), 0 deletions(-)
>>  create mode 100644 hw/spi.c
>>  create mode 100644 hw/spi.h
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index 44b2e83..8fd3718 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -320,6 +320,7 @@ obj-mips-$(CONFIG_FULONG) += bonito.o vt82c686.o mips_fulong2e.o
>>  obj-microblaze-y = petalogix_s3adsp1800_mmu.o
>>  obj-microblaze-y += petalogix_ml605_mmu.o
>>  obj-microblaze-y += microblaze_boot.o
>> +obj-microblaze-y += spi.o
>
> This should be in common-obj-y with i2c.o, I think -- microblaze isn't
> the only target with SPI (eg hw/omap_spi.c).
>

ok

>>  obj-microblaze-y += microblaze_pic_cpu.o
>>  obj-microblaze-y += xilinx_intc.o
>> diff --git a/hw/spi.c b/hw/spi.c
>> new file mode 100644
>> index 0000000..3afef07
>> --- /dev/null
>> +++ b/hw/spi.c
>> @@ -0,0 +1,175 @@
>> +/*
>> + * QEMU SPI bus interface.
>> + *
>> + * Copyright (C) 2012 Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> + * Copyright (C) 2012 PetaLogix
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "spi.h"
>> +
>> +static struct BusInfo spi_bus_info = {
>> +    .name = "SPI",
>> +    .size = sizeof(spi_bus)
>> +};
>> +
>> +static const VMStateDescription vmstate_spi_bus = {
>> +    .name = "spi_bus",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_UINT8(cur_slave, spi_bus),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +spi_bus *spi_init_bus(DeviceState *parent, int num_slaves, const char *name)
>> +{
>> +    spi_bus *bus;
>> +
>> +    bus = FROM_QBUS(spi_bus, qbus_create(&spi_bus_info, parent, name));
>> +    if (num_slaves >= SPI_BUS_NO_CS) {
>> +        hw_error("too many slaves on spi bus: %d\n", num_slaves);
>> +    }
>> +    bus->num_slaves = num_slaves;
>> +    bus->slaves = g_malloc0(sizeof(*bus->slaves) * num_slaves);
>> +    vmstate_register(NULL, -1, &vmstate_spi_bus, bus);
>> +    return bus;
>> +}
>> +
>> +int spi_attach_slave(spi_bus *bus, SPISlave *slave, int cs)
>> +{
>> +    if (bus->slaves[cs]) {
>> +        return 1;
>> +    }
>> +    bus->slaves[cs] = slave;
>> +    return 0;
>> +}
>> +
>> +int spi_set_cs(spi_bus *bus, int cs)
>> +{
>> +    SPISlave *dev;
>> +    SPISlaveClass *klass;
>> +
>> +    if (bus->cur_slave == cs) {
>> +        return 0;
>> +    }
>> +
>> +    if (bus->cur_slave != SPI_BUS_NO_CS) {
>> +        dev = bus->slaves[bus->cur_slave];
>> +        dev->cs = 0;
>> +        klass = SPI_SLAVE_GET_CLASS(dev);
>> +        klass->cs(dev, 0);
>> +    }
>> +
>> +    if (cs >= bus->num_slaves && cs != SPI_BUS_NO_CS) {
>> +        hw_error("attempted to assert non existent spi CS line: %d\n", cs);
>> +    }
>> +
>> +    bus->cur_slave = (uint8_t)cs;
>> +
>> +    if (cs != SPI_BUS_NO_CS) {
>> +        dev = bus->slaves[cs];
>> +        dev->cs = 1;
>> +        klass = SPI_SLAVE_GET_CLASS(dev);
>> +        klass->cs(dev, 1);
>> +    }
>> +    return 0;
>> +};
>> +
>> +int spi_get_cs(spi_bus *bus)
>> +{
>> +    return bus->cur_slave;
>> +}
>> +
>> +SpiSlaveState spi_get_state(spi_bus *bus)
>> +{
>> +    SPISlave *dev;
>> +    SPISlaveClass *klass;
>> +
>> +    if (bus->cur_slave == SPI_BUS_NO_CS) {
>> +        return SPI_NO_CS;
>> +    }
>> +    dev = bus->slaves[bus->cur_slave];
>> +    klass = SPI_SLAVE_GET_CLASS(dev);
>> +
>> +    return klass->get_state(dev);
>> +}
>> +
>> +SpiSlaveState spi_send(spi_bus *bus, uint32_t data, int len)
>> +{
>> +    SPISlave *dev;
>> +    SPISlaveClass *klass;
>> +
>> +    if (bus->cur_slave == SPI_BUS_NO_CS) {
>> +        return SPI_NO_CS;
>> +    }
>> +    dev = bus->slaves[bus->cur_slave];
>> +    klass = SPI_SLAVE_GET_CLASS(dev);
>> +
>> +    return klass->send(dev, data, len);
>> +}
>> +
>> +SpiSlaveState spi_recv(spi_bus *bus, uint32_t *data)
>> +{
>> +    SPISlave *dev;
>> +    SPISlaveClass *klass;
>> +
>> +    if (bus->cur_slave == SPI_BUS_NO_CS) {
>> +        return SPI_NO_CS;
>> +    }
>> +    dev = bus->slaves[bus->cur_slave];
>> +    klass = SPI_SLAVE_GET_CLASS(dev);
>> +
>> +    return klass->recv(dev, data);
>> +}
>> +
>> +const VMStateDescription vmstate_spi_slave = {
>> +    .name = "SPISlave",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_UINT8(cs, SPISlave),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static int spi_slave_qdev_init(DeviceState *dev)
>> +{
>> +    SPISlave *s = SPI_SLAVE_FROM_QDEV(dev);
>> +    SPISlaveClass *sc = SPI_SLAVE_GET_CLASS(s);
>> +
>> +    return sc->init(s);
>> +}
>> +
>> +static void spi_slave_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *k = DEVICE_CLASS(klass);
>> +    k->init = spi_slave_qdev_init;
>> +    k->bus_info = &spi_bus_info;
>> +}
>> +
>> +static TypeInfo spi_slave_type_info = {
>> +    .name = TYPE_SPI_SLAVE,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(SPISlave),
>> +    .abstract = true,
>> +    .class_size = sizeof(SPISlaveClass),
>> +    .class_init = spi_slave_class_init,
>> +};
>> +
>> +static void spi_slave_register_types(void)
>> +{
>> +    type_register_static(&spi_slave_type_info);
>> +}
>> +
>> +type_init(spi_slave_register_types)
>> diff --git a/hw/spi.h b/hw/spi.h
>> new file mode 100644
>> index 0000000..668e9b0
>> --- /dev/null
>> +++ b/hw/spi.h
>> @@ -0,0 +1,86 @@
>> +#ifndef QEMU_SPI_H
>> +#define QEMU_SPI_H
>> +
>> +#include "qdev.h"
>> +
>> +/* pass to spi_set_cs to deslect all devices on bus */
>
> "deselect"
>
>> +
>> +#define SPI_BUS_NO_CS 0xFF
>> +
>> +/* state of a SPI device,
>> + * SPI_NO_CS -> the CS pin in de-asserted -> device is tristated
>
> "is"
>

ack

>> + * SPI_IDLE -> CS is asserted and device ready to recv
>> + * SPI_DATA_PENDING -> CS is asserted and the device has pushed data to master
>> + */
>> +
>> +typedef enum {
>> +    SPI_NO_CS,
>> +    SPI_IDLE,
>> +    SPI_DATA_PENDING
>> +} SpiSlaveState;
>
> Why not SPISlaveState ?
>

ack

>> +
>> +typedef struct SPISlave {
>> +    DeviceState qdev;
>> +    uint8_t cs;
>> +} SPISlave;
>> +
>> +#define TYPE_SPI_SLAVE "spi-slave"
>> +#define SPI_SLAVE(obj) \
>> +     OBJECT_CHECK(SPISlave, (obj), TYPE_SPI_SLAVE)
>> +#define SPI_SLAVE_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(SPISlaveClass, (klass), TYPE_SPI_SLAVE)
>> +#define SPI_SLAVE_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(SPISlaveClass, (obj), TYPE_SPI_SLAVE)
>> +
>> +typedef struct SPISlaveClass {
>> +    DeviceClass parent_class;
>> +
>> +    /* Callbacks provided by the device.  */
>> +    int (*init)(SPISlave *s);
>> +
>> +    /* change the cs pin state */
>> +    void (*cs)(SPISlave *s, uint8_t select);
>> +
>> +    /* Master to slave.  */
>> +    SpiSlaveState (*send)(SPISlave *s, uint32_t data, int len);
>> +
>> +    /* Slave to master.  */
>> +    SpiSlaveState (*recv)(SPISlave *s, uint32_t *data);
>> +
>> +    /* poll the spi device state */
>> +    SpiSlaveState (*get_state)(SPISlave *s);
>> +} SPISlaveClass;
>> +
>> +#define SPI_SLAVE_FROM_QDEV(dev) DO_UPCAST(SPISlave, qdev, dev)
>> +#define FROM_SPI_SLAVE(type, dev) DO_UPCAST(type, spi, dev)
>> +
>> +extern const VMStateDescription vmstate_spi_slave;
>> +
>> +#define VMSTATE_SPI_SLAVE(_field, _state) {                          \
>> +    .name       = (stringify(_field)),                               \
>> +    .size       = sizeof(SPISlave),                                  \
>> +    .vmsd       = &vmstate_spi_slave,                                \
>> +    .flags      = VMS_STRUCT,                                        \
>> +    .offset     = vmstate_offset_value(_state, _field, SPISlave),    \
>> +}
>> +
>> +typedef struct spi_bus {
>> +    BusState qbus;
>> +    SPISlave **slaves;
>> +    uint8_t num_slaves;
>> +    uint8_t cur_slave;
>> +} spi_bus;
>
> CODING_STYLE demands camelcase for type names, so SPIBus.
>

Ok, I have a related question tho, if camel casing with acronyms,
should a space perhaps be inserted after for readability? As in
SPI_Bus rather than SPIBus.

>> +
>> +/* create a new spi bus */
>> +spi_bus *spi_init_bus(DeviceState *parent, int num_slaves, const char *name);
>> +int spi_attach_slave(spi_bus *bus, SPISlave *s, int cs);
>> +
>> +/* change the chip select. Return 1 on failure. */
>> +int spi_set_cs(spi_bus *bus, int cs);
>> +int spi_get_cs(spi_bus *bus);
>> +SpiSlaveState spi_get_state(spi_bus *bus);
>> +
>> +SpiSlaveState spi_send(spi_bus *bus, uint32_t data, int len);
>> +SpiSlaveState spi_recv(spi_bus *bus, uint32_t *data);
>
> I'm no SPI expert, but a bit of googling suggests that it's
> a synchronous duplex bus, so you always send a byte of data
> to the slave and get one back in return (even if for some slaves
> it might be random garbage). The current OMAP SPI devices in QEMU
> master have an API that reflects this: eg tsc210x_txrx() which
> takes an input value and returns an output value. This API
> seems to have separate send and recv methods -- can you explain
> how this works?
>
> (Incidentally if we're going to qdevify the SPI interface we
> should also convert the existing omap SPI devices...)
>
> thanks
> -- PMM

Am 03.04.2012 01:57, schrieb Peter Crosthwaite:
> On Tue, Apr 3, 2012 at 3:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 30 March 2012 07:37, Peter A. G. Crosthwaite
>> <peter.crosthwaite@petalogix.com> wrote:
>>> +typedef struct spi_bus {
>>> +    BusState qbus;
>>> +    SPISlave **slaves;
>>> +    uint8_t num_slaves;
>>> +    uint8_t cur_slave;
>>> +} spi_bus;
>>
>> CODING_STYLE demands camelcase for type names, so SPIBus.
>>
> 
> Ok, I have a related question tho, if camel casing with acronyms,
> should a space perhaps be inserted after for readability? As in
> SPI_Bus rather than SPIBus.

Negative, we have PCIDevice, I2CSlave, etc.

The underscore is used in macro names, such as X86_CPU() vs. X86CPU.
If we allow underscores in type names as word separator then sooner or
later we'll run into the same ugly uppercase name conflicts that caused
the huge'ish CPUState refactoring.

.NET avoids four consecutive uppercase letter by writing, e.g.,
XmlDocument. So far we have rather adopted the older (e.g., Java) model
of not lower-casing acronyms. My preference would be for consistency.

Andreas

Hi Peter,

So ive looked into this further, and I think I better see the
confusion between us. My api attempt is attempting to abstract away
the clock wheras the txrx approach is true'er to the physical
interface. This sense of one-sided communication that im trying to
model is comming from the fact that device can drive their SDO line to
tristate, which effiectly means "no-data". To that end, a compromise
between the two API would be to modify the txrx API to allow capturing
of this information:

int spi_txrx(uint8_t *tx, uint8_t *txz, uint8_t *rx, uint8_t *rxz,  int len);

tx and rx and arbitary length bit-buffers for tx and rx data. txz and
rxz and parrell to their two respective buffers, and indicate whether
the line is in tristate, i.e. between the two arrays you can infer
whether the line is 0, 1 or Z. Len then exactly corresponds to the
number of clock edges in the transaction. This would allow
encapsulation of all the desired behaviour with minimal modification
to the txrx interface and no hacky escape return behaviours.

What your think?

Regards
Peter


On Tue, Apr 3, 2012 at 9:51 AM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
>>> +
>>> +/* create a new spi bus */
>>> +spi_bus *spi_init_bus(DeviceState *parent, int num_slaves, const char *name);
>>> +int spi_attach_slave(spi_bus *bus, SPISlave *s, int cs);
>>> +
>>> +/* change the chip select. Return 1 on failure. */
>>> +int spi_set_cs(spi_bus *bus, int cs);
>>> +int spi_get_cs(spi_bus *bus);
>>> +SpiSlaveState spi_get_state(spi_bus *bus);
>>> +
>>> +SpiSlaveState spi_send(spi_bus *bus, uint32_t data, int len);
>>> +SpiSlaveState spi_recv(spi_bus *bus, uint32_t *data);
>>
>> I'm no SPI expert, but a bit of googling suggests that it's
>> a synchronous duplex bus, so you always send a byte of data
>> to the slave and get one back in return (even if for some slaves
>> it might be random garbage).
>
> Hi Peter,
>
> Thats not correct, SPI txs can trigger rxs of arbitrary length. Heres
> the data sheet for the m25p80 spi flash device:
> www.datasheetcatalog.org/datasheet/stmicroelectronics/8495.pdf.
> Figure 12, page 15 is a good example of this, the master transmits 4
> bytes back to back for a opcode and an addresses, and the slave bursts
> the entire playload back. To hack around this, Edgar had to put escape
> sequences in that txrx api to flag all the txs and rxs for devices and
> controllers respecitvely, ive attached a file (xilinx_spi.h) which is
> Eds original hack of the txrx interface that im trying to fix here by
> changing the API.
>
> This api allows controllers to manage that without dummy txs or invalid rxs.
>
> The current OMAP SPI devices in QEMU
>> master have an API that reflects this: eg tsc210x_txrx() which
>> takes an input value and returns an output value. This API
>> seems to have separate send and recv methods -- can you explain
>> how this works?
>>
>
> Each send or receive command will return a enum SPISlaveState. If
> SPI_DATA_PENDING the master can receive a byte back from the slave
> with recv. If that recv returns SPI_DATA_PENDING then the master
> should recieve again i.e. recv should be looped. Heres how its handled
> in the xilinx_spi controller:
>
>    uint32_t value;
>    SpiSlaveState st;
>
>    while (!txfifo_empty(s)) {
>        value = txfifo_get(s);
>        DB_PRINT("data transfer:%x\n", value);
>        st = spi_send(s->spi, value, 8);
>        while (st == SPI_DATA_PENDING) {
>            uint32_t d;
>            st = spi_recv(s->spi, &d);
>            rxfifo_put(s, d);
>        }
>    }
>
> The inner loop is basically a poll, waiting for the recv to return !=
> SPI_DATA_PENDING.
>
>> (Incidentally if we're going to qdevify the SPI interface we
>> should also convert the existing omap SPI devices...)
>
> Yes I havent looked into the details, but shouldnt be too much work. I
> can provide the diff for the m25p80 to change it over from txrx style
> to this, to give an idea of the change pattern. Little bit of this
> that and the other in the machine models too.
>
> If we really want to minimise work, the old txrx function can be added
> (to spi.c) as something like:
>
> uint32_t spi_txrx(spi_bus *bus, uint32_t data) {
>      uint32_t ret;
>
>      assert(spi_send(bus, data) == SPI_DATA_PENDING);
>      assert(spi_recv(bus, &ret) == SPI_DATA_IDLE);
>      return ret;
> }
>
>>
>> thanks
>> -- PMM
>
> Regards,
> Peter

> > I'm no SPI expert, but a bit of googling suggests that it's
> > a synchronous duplex bus, so you always send a byte of data
> > to the slave and get one back in return (even if for some slaves
> > it might be random garbage).

Waitaminute. So this is just basic synchronous serial?

We already have an API for this. hw/spi.c.

If you're building higer level protocols they should be layered on top of the 
SPI, same as smbus is a protocol layer on top of I2C.

Paul

2012/4/3 Paul Brook <paul@codesourcery.com>:
>> > I'm no SPI expert, but a bit of googling suggests that it's
>> > a synchronous duplex bus, so you always send a byte of data
>> > to the slave and get one back in return (even if for some slaves
>> > it might be random garbage).
>
> Waitaminute. So this is just basic synchronous serial?
>
> We already have an API for this. hw/spi.c.

Er, no we don't, not in upstream QEMU. You'll notice that
the patch we're discussing creates hw/spi.c as a new file...

-- PMM

> 2012/4/3 Paul Brook <paul@codesourcery.com>:
> >> > I'm no SPI expert, but a bit of googling suggests that it's
> >> > a synchronous duplex bus, so you always send a byte of data
> >> > to the slave and get one back in return (even if for some slaves
> >> > it might be random garbage).
> > 
> > Waitaminute. So this is just basic synchronous serial?
> > 
> > We already have an API for this. hw/spi.c.
> 
> Er, no we don't, not in upstream QEMU. You'll notice that
> the patch we're discussing creates hw/spi.c as a new file...

Sorry, I mean hw/ssi.c

Synchronous serial is something that pretty much every vendor has their own 
name for, but in practice they're actually all the same.

Paul

Hi Paul,

Regarding using ssi, theres a few things that come to mind:

Theres no sense of it being a multi-slave bus, its just a point to
point link. SPI devices universally have the notion of the CS pin that
tristates the device of the bus. Masters connect to a number of slaves
and one-hot-decode the active slave. It would be tedious if all SPI
controllers (of which i have two (only pushed one in this series),
omap have theirs, Nokia may have something out of tree too), had to
implement the\is common CS decoding behaviour. To that end, my SPI bus
implementation has some infra-structure for managing this -
spi_set_cs(). The spi_bus in this series is single master,
multi-slave, where as ssi is point-to-point.

Also as mentioned in earlier discussions with Peter, that api has no
way of emulating the CS (sometimes called SS) pin. The m25p80 in this
series (and potenitally other devices) has side effect associated with
wiggling the cs pin, which can not be encapsulated by that ssi slave
interface.

It may be a case though that ssi is a superclass of spi - all SPI
devices are SSI devices but not all SSI devices are SPI? To that end
can we make SPI a child object of SSI with the desired extra behaviors
mentioned in this thread? This kind of stuff is the whole reason for
QOM.

Regards,
Peter

On Wed, Apr 4, 2012 at 7:22 AM, Paul Brook <paul@codesourcery.com> wrote:
>> 2012/4/3 Paul Brook <paul@codesourcery.com>:
>> >> > I'm no SPI expert, but a bit of googling suggests that it's
>> >> > a synchronous duplex bus, so you always send a byte of data
>> >> > to the slave and get one back in return (even if for some slaves
>> >> > it might be random garbage).
>> >
>> > Waitaminute. So this is just basic synchronous serial?
>> >
>> > We already have an API for this. hw/spi.c.
>>
>> Er, no we don't, not in upstream QEMU. You'll notice that
>> the patch we're discussing creates hw/spi.c as a new file...
>
> Sorry, I mean hw/ssi.c
>
> Synchronous serial is something that pretty much every vendor has their own
> name for, but in practice they're actually all the same.
>
> Paul

> Hi Paul,
> 
> Regarding using ssi, theres a few things that come to mind:
> 
> Theres no sense of it being a multi-slave bus, its just a point to
> point link. SPI devices universally have the notion of the CS pin that
> tristates the device of the bus. Masters connect to a number of slaves
> and one-hot-decode the active slave. It would be tedious if all SPI
> controllers (of which i have two (only pushed one in this series),
> omap have theirs, Nokia may have something out of tree too), had to
> implement the\is common CS decoding behaviour. To that end, my SPI bus
> implementation has some infra-structure for managing this -
> spi_set_cs(). The spi_bus in this series is single master,
> multi-slave, where as ssi is point-to-point.
> 
> Also as mentioned in earlier discussions with Peter, that api has no
> way of emulating the CS (sometimes called SS) pin. The m25p80 in this
> series (and potenitally other devices) has side effect associated with
> wiggling the cs pin, which can not be encapsulated by that ssi slave
> interface.
> 
> It may be a case though that ssi is a superclass of spi - all SPI
> devices are SSI devices but not all SSI devices are SPI? To that end
> can we make SPI a child object of SSI with the desired extra behaviors
> mentioned in this thread? This kind of stuff is the whole reason for
> QOM.

I don't believe there is any difference between SSI and SPI.  It's the exact 
same thing - the same way that many devices support a "two-wire interface" 
that is actually just I2C with a different name.

The behavior of the CS pin varies between devices.  It sounds like you need a 
bit of extra logic not present in the current ssi code.  You should fix that, 
not invent a whole new bus.

Paul