Re: [Qemu-devel] [RFC PATCH v1 1/4] SPI: initial support

[Peter Crosthwaite]

On Tue, Apr 3, 2012 at 3:39 AM, Peter Maydell <address@hidden> wrote:
> On 30 March 2012 07:37, Peter A. G. Crosthwaite
> <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>
>> + * 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