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

[Peter Crosthwaite]

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
<address@hidden> 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