[Qemu-devel] [PATCH v2 0/4] Add support for i.MX SPI Controller

[Qemu-devel] [PATCH v2 0/4] Add support for i.MX SPI Controller

[Jean-Christophe Dubois]

This patch serie adds support for the i.MX SPI controller.

The controller is then added to the i.MX6 SOC implementation to
avoid Linux hang during boot because there is no support for the SPI
controller and it is waiting for some SPI device response.

We also add a NOR FLASH SPI memory to the sabrelite board to test the
SPI implementation.

This patch also adds a FIFO32 implementation as it is part of the
i.MX SPI controller basic mechanisms. The FIFO32 is build on top of
the existing FIFO8.

Jean-Christophe Dubois (4):
  FIFO: Add a FIFO32 implementation
  i.MX: Add the Freescale SPI Controller
  i.MX: Add SPI controllers to i.MX6 SOC
  i.MX: Add SPI NOR FLASH memory to sabrelite board.

 hw/arm/fsl-imx6.c         |  32 ++++
 hw/arm/sabrelite.c        |   9 +
 hw/ssi/Makefile.objs      |   1 +
 hw/ssi/imx_spi.c          | 459 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/fsl-imx6.h |   3 +
 include/hw/ssi/imx_spi.h  | 104 +++++++++++
 include/qemu/fifo32.h     | 206 +++++++++++++++++++++
 7 files changed, 814 insertions(+)
 create mode 100644 hw/ssi/imx_spi.c
 create mode 100644 include/hw/ssi/imx_spi.h
 create mode 100644 include/qemu/fifo32.h

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/4] FIFO: Add a FIFO32 implementation

[Jean-Christophe Dubois]

This one is build on top of the existing FIFO8

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---

Changes since v1:
 * None

 include/qemu/fifo32.h | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 206 insertions(+)
 create mode 100644 include/qemu/fifo32.h

diff --git a/include/qemu/fifo32.h b/include/qemu/fifo32.h
new file mode 100644
index 0000000..d934516
--- /dev/null
+++ b/include/qemu/fifo32.h
@@ -0,0 +1,206 @@
+#ifndef FIFO32_H
+#define FIFO32_H
+
+#include "qemu/fifo8.h"
+
+typedef Fifo8 Fifo32;
+
+/**
+ * fifo32_create:
+ * @fifo: struct Fifo32 to initialise with new FIFO
+ * @capacity: capacity of the newly created FIFO
+ *
+ * Create a FIFO of the specified size. Clients should call fifo32_destroy()
+ * when finished using the fifo. The FIFO is initially empty.
+ */
+
+static inline void fifo32_create(Fifo32 *fifo, uint32_t capacity)
+{
+    fifo8_create(fifo, capacity * sizeof(uint32_t));
+}
+
+/**
+ * fifo32_destroy:
+ * @fifo: FIFO to cleanup
+ *
+ * Cleanup a FIFO created with fifo32_create(). Frees memory created for FIFO
+  *storage. The FIFO is no longer usable after this has been called.
+ */
+
+static inline void fifo32_destroy(Fifo32 *fifo)
+{
+    fifo8_destroy(fifo);
+}
+
+/**
+ * fifo32_num_free:
+ * @fifo: FIFO to check
+ *
+ * Return the number of free uint32_t slots in the FIFO.
+ *
+ * Returns: Number of free bytes.
+ */
+
+static inline uint32_t fifo32_num_free(Fifo32 *fifo)
+{
+    return (fifo8_num_free(fifo) + sizeof(uint32_t) - 1) / sizeof(uint32_t);
+}
+
+/**
+ * fifo32_num_used:
+ * @fifo: FIFO to check
+ *
+ * Return the number of used uint32_t slots in the FIFO.
+ *
+ * Returns: Number of used bytes.
+ */
+
+static inline uint32_t fifo32_num_used(Fifo32 *fifo)
+{
+    return (fifo8_num_used(fifo) + sizeof(uint32_t) - 1) / sizeof(uint32_t);
+}
+
+/**
+ * fifo32_push:
+ * @fifo: FIFO to push to
+ * @data: data byte to push
+ *
+ * Push a data byte to the FIFO. Behaviour is undefined if the FIFO is full.
+ * Clients are responsible for checking for fullness using fifo32_is_full().
+ */
+
+static inline void fifo32_push(Fifo32 *fifo, uint32_t data)
+{
+    uint8_t *ptr = (uint8_t *)&data;
+    int i;
+
+    if (fifo32_num_free(fifo) == 0) {
+        abort();
+    }
+
+    for (i=0; i < sizeof(data); i++) {
+        fifo8_push(fifo, ptr[i]);
+    }
+}
+
+/**
+ * fifo32_push_all:
+ * @fifo: FIFO to push to
+ * @data: data to push
+ * @size: number of bytes to push
+ *
+ * Push a byte array to the FIFO. Behaviour is undefined if the FIFO is full.
+ * Clients are responsible for checking the space left in the FIFO using
+ * fifo32_num_free().
+ */
+
+static inline void fifo32_push_all(Fifo32 *fifo, const uint32_t *data,
+                                   uint32_t num)
+{
+    fifo8_push_all(fifo, (const uint8_t *)data, num * sizeof(uint32_t));
+}
+
+/**
+ * fifo32_pop:
+ * @fifo: fifo to pop from
+ *
+ * Pop a data byte from the FIFO. Behaviour is undefined if the FIFO is empty.
+ * Clients are responsible for checking for emptyness using fifo32_is_empty().
+ *
+ * Returns: The popped data byte.
+ */
+
+static inline uint32_t fifo32_pop(Fifo32 *fifo)
+{
+    uint32_t ret = 0;
+    uint8_t *ptr = (uint8_t *)&ret;
+    int i;
+
+    for (i=0; i < sizeof(uint32_t); i++) {
+        if (fifo8_is_empty(fifo)) {
+            break;
+        }
+        ptr[i] = fifo8_pop(fifo);
+    }
+
+    return ret;
+}
+
+/**
+ * fifo32_pop_buf:
+ * @fifo: FIFO to pop from
+ * @max: maximum number of bytes to pop
+ * @num: actual number of returned bytes
+ *
+ * Pop a number of elements from the FIFO up to a maximum of max. The buffer
+ * containing the popped data is returned. This buffer points directly into
+ * the FIFO backing store and data is invalidated once any of the fifo32_* APIs
+ * are called on the FIFO.
+ *
+ * The function may return fewer bytes than requested when the data wraps
+ * around in the ring buffer; in this case only a contiguous part of the data
+ * is returned.
+ *
+ * The number of valid bytes returned is populated in *num; will always return
+ * at least 1 byte. max must not be 0 or greater than the number of bytes in
+ * the FIFO.
+ *
+ * Clients are responsible for checking the availability of requested data
+ * using fifo32_num_used().
+ *
+ * Returns: A pointer to popped data.
+ */
+
+static inline const uint32_t *fifo32_pop_buf(Fifo32 *fifo, uint32_t max,
+                                             uint32_t *num)
+{
+    const uint8_t *ptr = fifo8_pop_buf(fifo, max * sizeof(uint32_t), num);
+
+    *num /= sizeof(uint32_t);
+
+    return (const uint32_t *)ptr;
+}
+
+/**
+ * fifo32_reset:
+ * @fifo: FIFO to reset
+ *
+ * Reset a FIFO. All data is discarded and the FIFO is emptied.
+ */
+
+static inline void fifo32_reset(Fifo32 *fifo)
+{
+    fifo8_reset(fifo);
+}
+
+/**
+ * fifo32_is_empty:
+ * @fifo: FIFO to check
+ *
+ * Check if a FIFO is empty.
+ *
+ * Returns: True if the fifo is empty, false otherwise.
+ */
+
+static inline bool fifo32_is_empty(Fifo32 *fifo)
+{
+    return fifo8_is_empty(fifo);
+}
+
+/**
+ * fifo32_is_full:
+ * @fifo: FIFO to check
+ *
+ * Check if a FIFO is full.
+ *
+ * Returns: True if the fifo is full, false otherwise.
+ */
+
+static inline bool fifo32_is_full(Fifo32 *fifo)
+{
+    return fifo8_num_free(fifo) < sizeof(uint32_t) ? true : false;
+}
+
+#define VMSTATE_FIFO32(_field, _state) VMSTATE_FIFO8(_field, _state)
+
+#endif /* FIFO32_H */
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 1/4] FIFO: Add a FIFO32 implementation

[Peter Maydell]

On 15 February 2016 at 11:17, Jean-Christophe Dubois
<jcd@tribudubois.net> wrote:
> This one is build on top of the existing FIFO8
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>

> --- /dev/null
> +++ b/include/qemu/fifo32.h
> @@ -0,0 +1,206 @@

Can we have a copyright/license comment at the top of new files, please?
(I know fifo8.h doesn't have one, but ideally it should.)
Use gpl-2-or-later, since that's what util/fifo.c does and thus
by extension what Peter C intended for the header too.

> +#ifndef FIFO32_H
> +#define FIFO32_H
> +
> +#include "qemu/fifo8.h"
> +
> +typedef Fifo8 Fifo32;

This means the compiler won't notice if you accidentally pass
a Fifo8 to a fifo32 function, which seems a shame. Could we do

typedef struct {
    Fifo8 bytefifo;
} Fifo32;

(and then have all these wrapper functions pass &fifo->bytefifo to
the underlying fifo8 functions) ?

> +
> +/**
> + * fifo32_create:
> + * @fifo: struct Fifo32 to initialise with new FIFO
> + * @capacity: capacity of the newly created FIFO

You should specifically say "(number of 32-bit words)" here to avoid
possible confusion.

> + *
> + * Create a FIFO of the specified size. Clients should call fifo32_destroy()
> + * when finished using the fifo. The FIFO is initially empty.
> + */
> +
> +static inline void fifo32_create(Fifo32 *fifo, uint32_t capacity)
> +{
> +    fifo8_create(fifo, capacity * sizeof(uint32_t));
> +}
> +
> +/**
> + * fifo32_destroy:
> + * @fifo: FIFO to cleanup
> + *
> + * Cleanup a FIFO created with fifo32_create(). Frees memory created for FIFO
> +  *storage. The FIFO is no longer usable after this has been called.

Space has got into the wrong place at start of this line.

> + */
> +
> +static inline void fifo32_destroy(Fifo32 *fifo)
> +{
> +    fifo8_destroy(fifo);
> +}
> +
> +/**
> + * fifo32_num_free:
> + * @fifo: FIFO to check
> + *
> + * Return the number of free uint32_t slots in the FIFO.
> + *
> + * Returns: Number of free bytes.
> + */
> +
> +static inline uint32_t fifo32_num_free(Fifo32 *fifo)
> +{
> +    return (fifo8_num_free(fifo) + sizeof(uint32_t) - 1) / sizeof(uint32_t);

Why the round-up code? Isn't fifo8_num_free() always going to
return a multiple of 4 ? (If it is necessary, this is
DIV_ROUND_UP(fifo8_num_free(fifo), sizeof(uint32_t)) .)

> +}
> +
> +/**
> + * fifo32_num_used:
> + * @fifo: FIFO to check
> + *
> + * Return the number of used uint32_t slots in the FIFO.
> + *
> + * Returns: Number of used bytes.
> + */
> +
> +static inline uint32_t fifo32_num_used(Fifo32 *fifo)
> +{
> +    return (fifo8_num_used(fifo) + sizeof(uint32_t) - 1) / sizeof(uint32_t);

Similarly here.

> +}
> +
> +/**
> + * fifo32_push:
> + * @fifo: FIFO to push to
> + * @data: data byte to push

Not a byte...

> + *
> + * Push a data byte to the FIFO. Behaviour is undefined if the FIFO is full.

Ditto.

> + * Clients are responsible for checking for fullness using fifo32_is_full().
> + */
> +
> +static inline void fifo32_push(Fifo32 *fifo, uint32_t data)
> +{
> +    uint8_t *ptr = (uint8_t *)&data;

This is a bad idea. It means the representation of the data in the
underlying buffer depends on the endianness of the host system, which
means that migration between big and little endian hosts won't work.

Unfortunately fifo32_pop_buf() as an API makes this awkward,
because it implicitly asusmes that the buffer has data as
a series of host 32-bit words.

I think we have two choices:
(1) drop fifo32_pop_buf(), and store data in the fifo in
a fixed order, like LE:
    uint8_t le_data[4];
    stl_le_p(le_data, data);
    for (i = 0; i < sizeof(le_data); i++) {
        fifo8_push(fifo, le_data[i]);
    }
(and equivalently ldl_le_p() on fifo pop)

(2) store data in the fifo in host-word order and fix the
migration handling to cope with saving and restoring buffers
that need a 'swap from BE to host order' pass.

(2) would be cleaner but is more effort grubbing around in
the migration code. Unfortunately we can't start with the easy
choice and switch later if we need to because it would be a
migration compatibility break, so I think we need to do the
difficult but right thing now.

> +    int i;
> +
> +    if (fifo32_num_free(fifo) == 0) {
> +        abort();
> +    }

You don't need this, you can let fifo8_push() handle it for you.

> +
> +    for (i=0; i < sizeof(data); i++) {
> +        fifo8_push(fifo, ptr[i]);
> +    }
> +}
> +
> +/**
> + * fifo32_push_all:
> + * @fifo: FIFO to push to
> + * @data: data to push
> + * @size: number of bytes to push
> + *
> + * Push a byte array to the FIFO.

More things saying "byte" that don't mean it.

> Behaviour is undefined if the FIFO is full.
> + * Clients are responsible for checking the space left in the FIFO using
> + * fifo32_num_free().
> + */
> +
> +static inline void fifo32_push_all(Fifo32 *fifo, const uint32_t *data,
> +                                   uint32_t num)
> +{
> +    fifo8_push_all(fifo, (const uint8_t *)data, num * sizeof(uint32_t));

This will bite us if 'num' is guest-controlled information because then
the guest could make the multiply overflow.

> +}
> +
> +/**
> + * fifo32_pop:
> + * @fifo: fifo to pop from
> + *
> + * Pop a data byte from the FIFO. Behaviour is undefined if the FIFO is empty.

not "byte"

> + * Clients are responsible for checking for emptyness using fifo32_is_empty().

"emptiness"

> + *
> + * Returns: The popped data byte.
> + */
> +
> +static inline uint32_t fifo32_pop(Fifo32 *fifo)
> +{
> +    uint32_t ret = 0;
> +    uint8_t *ptr = (uint8_t *)&ret;
> +    int i;
> +
> +    for (i=0; i < sizeof(uint32_t); i++) {
> +        if (fifo8_is_empty(fifo)) {
> +            break;

This makes the behaviour on empty fifo different from fifo8_pop(),
which just abort()s. You can drop this check and let fifo8_pop()
handle the empty fifo case.

> +        }
> +        ptr[i] = fifo8_pop(fifo);
> +    }
> +
> +    return ret;
> +}
> +
> +/**
> + * fifo32_pop_buf:
> + * @fifo: FIFO to pop from
> + * @max: maximum number of bytes to pop
> + * @num: actual number of returned bytes

not "bytes"

> + *
> + * Pop a number of elements from the FIFO up to a maximum of max. The buffer
> + * containing the popped data is returned. This buffer points directly into
> + * the FIFO backing store and data is invalidated once any of the fifo32_* APIs
> + * are called on the FIFO.
> + *
> + * The function may return fewer bytes than requested when the data wraps
> + * around in the ring buffer; in this case only a contiguous part of the data
> + * is returned.
> + *
> + * The number of valid bytes returned is populated in *num; will always return
> + * at least 1 byte. max must not be 0 or greater than the number of bytes in
> + * the FIFO.
> + *
> + * Clients are responsible for checking the availability of requested data
> + * using fifo32_num_used().
> + *
> + * Returns: A pointer to popped data.
> + */
> +
> +static inline const uint32_t *fifo32_pop_buf(Fifo32 *fifo, uint32_t max,
> +                                             uint32_t *num)
> +{
> +    const uint8_t *ptr = fifo8_pop_buf(fifo, max * sizeof(uint32_t), num);
> +
> +    *num /= sizeof(uint32_t);
> +
> +    return (const uint32_t *)ptr;

You can't just cast this because you have no guarantee that the
value you get back from fifo8_pop_buf() meets the alignment
restrictions for a uint32_t*.

> +}
> +
> +/**
> + * fifo32_reset:
> + * @fifo: FIFO to reset
> + *
> + * Reset a FIFO. All data is discarded and the FIFO is emptied.
> + */
> +
> +static inline void fifo32_reset(Fifo32 *fifo)
> +{
> +    fifo8_reset(fifo);
> +}
> +
> +/**
> + * fifo32_is_empty:
> + * @fifo: FIFO to check
> + *
> + * Check if a FIFO is empty.
> + *
> + * Returns: True if the fifo is empty, false otherwise.
> + */
> +
> +static inline bool fifo32_is_empty(Fifo32 *fifo)
> +{
> +    return fifo8_is_empty(fifo);
> +}
> +
> +/**
> + * fifo32_is_full:
> + * @fifo: FIFO to check
> + *
> + * Check if a FIFO is full.
> + *
> + * Returns: True if the fifo is full, false otherwise.
> + */
> +
> +static inline bool fifo32_is_full(Fifo32 *fifo)
> +{
> +    return fifo8_num_free(fifo) < sizeof(uint32_t) ? true : false;

 "X ? true : false" is a longwinded way to say "X".

> +}
> +
> +#define VMSTATE_FIFO32(_field, _state) VMSTATE_FIFO8(_field, _state)
> +
> +#endif /* FIFO32_H */

thanks
-- PMM

[Qemu-devel] [PATCH v2 2/4] i.MX: Add the Freescale SPI Controller

[Jean-Christophe Dubois]

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---

Changes since v1:
 * Access SPI slave only at a byte level.
 * rework the CS activation to avoid to reset access to SPI slaves.

 hw/ssi/Makefile.objs     |   1 +
 hw/ssi/imx_spi.c         | 459 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ssi/imx_spi.h | 104 +++++++++++
 3 files changed, 564 insertions(+)
 create mode 100644 hw/ssi/imx_spi.c
 create mode 100644 include/hw/ssi/imx_spi.h

diff --git a/hw/ssi/Makefile.objs b/hw/ssi/Makefile.objs
index 9555825..fcbb79e 100644
--- a/hw/ssi/Makefile.objs
+++ b/hw/ssi/Makefile.objs
@@ -4,3 +4,4 @@ common-obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 common-obj-$(CONFIG_XILINX_SPIPS) += xilinx_spips.o
 
 obj-$(CONFIG_OMAP) += omap_spi.o
+obj-$(CONFIG_IMX) += imx_spi.o
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
new file mode 100644
index 0000000..cab102c
--- /dev/null
+++ b/hw/ssi/imx_spi.c
@@ -0,0 +1,459 @@
+/*
+ * IMX SPI Controller
+ *
+ * Copyright (c) 2016 Jean-Christophe Dubois <jcd@tribudubois.net>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/ssi/imx_spi.h"
+#include "sysemu/sysemu.h"
+
+#ifndef DEBUG_IMX_SPI
+#define DEBUG_IMX_SPI 0
+#endif
+
+#define DPRINTF(fmt, args...) \
+    do { \
+        if (DEBUG_IMX_SPI) { \
+            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SPI, \
+                                             __func__, ##args); \
+        } \
+    } while (0)
+
+static char const *imx_spi_reg_name(uint32_t reg)
+{
+    static char unknown[20];
+
+    switch (reg) {
+    case ECSPI_RXDATA:
+        return  "ECSPI_RXDATA";
+    case ECSPI_TXDATA:
+        return  "ECSPI_TXDATA";
+    case ECSPI_CONREG:
+        return  "ECSPI_CONREG";
+    case ECSPI_CONFIGREG:
+        return  "ECSPI_CONFIGREG";
+    case ECSPI_INTREG:
+        return  "ECSPI_INTREG";
+    case ECSPI_DMAREG:
+        return  "ECSPI_DMAREG";
+    case ECSPI_STATREG:
+        return  "ECSPI_STATREG";
+    case ECSPI_PERIODREG:
+        return  "ECSPI_PERIODREG";
+    case ECSPI_TESTREG:
+        return  "ECSPI_TESTREG";
+    case ECSPI_MSGDATA:
+        return  "ECSPI_MSGDATA";
+    default:
+        sprintf(unknown, "%d ?", reg);
+        return unknown;
+    }
+}
+
+static const VMStateDescription vmstate_imx_spi = {
+    .name = TYPE_IMX_SPI,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_FIFO32(tx_fifo, IMXSPIState),
+        VMSTATE_FIFO32(rx_fifo, IMXSPIState),
+        VMSTATE_UINT32_ARRAY(regs, IMXSPIState, ECSPI_MAX),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void imx_spi_txfifo_reset(IMXSPIState *s)
+{
+    fifo32_reset(&s->tx_fifo);
+    s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
+    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
+}
+
+static void imx_spi_rxfifo_reset(IMXSPIState *s)
+{
+    fifo32_reset(&s->rx_fifo);
+    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
+    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
+    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RO;
+}
+
+static void imx_spi_update_irq(IMXSPIState *s)
+{
+    int level;
+
+    if (fifo32_is_empty(&s->rx_fifo)) {
+        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
+    } else {
+        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RR;
+    }
+
+    if (fifo32_is_full(&s->rx_fifo)) {
+        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RF;
+    } else {
+        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
+    }
+
+    if (fifo32_is_empty(&s->tx_fifo)) {
+        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
+    } else {
+        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TE;
+    }
+
+    if (fifo32_is_full(&s->tx_fifo)) {
+        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TF;
+    } else {
+        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
+    }
+
+    level = s->regs[ECSPI_STATREG] & s->regs[ECSPI_INTREG] ? 1 : 0;
+
+    if (s->previous_level != level) {
+        DPRINTF("setting IRQ a level %d\n", level);
+        s->previous_level = level;
+        qemu_set_irq(s->irq, level);
+    }
+
+    DPRINTF("IRQ level is %d\n", level);
+}
+
+static uint8_t imx_spi_selected_channel(IMXSPIState *s)
+{
+    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_CHANNEL_SELECT);
+}
+
+static uint32_t imx_spi_burst_length(IMXSPIState *s)
+{
+    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
+}
+
+static bool imx_spi_is_enabled(IMXSPIState *s)
+{
+    return (s->regs[ECSPI_CONREG] & ECSPI_CONREG_EN) ? true : false;
+}
+
+static bool imx_spi_channel_is_master(IMXSPIState *s)
+{
+    uint8_t mode = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_CHANNEL_MODE);
+
+    return (mode & (1 << imx_spi_selected_channel(s))) ? true : false;
+}
+
+static bool imx_spi_is_multiple_master_burst(IMXSPIState *s)
+{
+    uint8_t wave = EXTRACT(s->regs[ECSPI_CONFIGREG], ECSPI_CONFIGREG_SS_CTL);
+
+    return imx_spi_channel_is_master(s) &&
+           !(s->regs[ECSPI_CONREG] & ECSPI_CONREG_SMC) &&
+           ((wave & (1 << imx_spi_selected_channel(s))) ? true : false);
+}
+
+static void imx_spi_flush_txfifo(IMXSPIState *s)
+{
+    uint32_t tx;
+    uint32_t rx;
+
+    DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n",
+            fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
+
+    while (!fifo32_is_empty(&s->tx_fifo)) {
+        int tx_burst = 0;
+
+        if (s->burst_length <= 0) {
+            s->burst_length = imx_spi_burst_length(s);
+
+            DPRINTF("Burst length = %d\n", s->burst_length);
+
+            if (imx_spi_is_multiple_master_burst(s)) {
+                s->regs[ECSPI_CONREG] |= ECSPI_CONREG_XCH;
+            }
+        }
+
+        tx = fifo32_pop(&s->tx_fifo);
+
+        DPRINTF("data tx:0x%08x\n", tx);
+
+        tx_burst = MIN(s->burst_length, 32);
+
+        rx = 0;
+
+        while (tx_burst) {
+            rx = rx << 8;
+
+            DPRINTF("writing 0x%02x\n", tx & 0xff);
+
+            /* We need to write one byte at a time */
+            rx |= ssi_transfer(s->bus, tx & 0xFF) & 0xFF;
+
+            DPRINTF("0x%02x read\n", rx & 0xff);
+
+            tx = tx >> 8;
+
+            /* Remove 8 bits from the actual burst */
+            tx_burst -= 8;
+
+            s->burst_length -= 8;
+        }
+
+        DPRINTF("data rx:0x%08x\n", rx);
+
+        if (fifo32_is_full(&s->rx_fifo)) {
+            s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
+        } else {
+            fifo32_push(&s->rx_fifo, (uint8_t)rx);
+        }
+
+        if (s->burst_length <= 0) {
+            s->regs[ECSPI_CONREG] &= ~ECSPI_CONREG_XCH;
+
+            if (!imx_spi_is_multiple_master_burst(s)) {
+                s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
+                break;
+            }
+        }
+    }
+
+    if (fifo32_is_empty(&s->tx_fifo)) {
+        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
+    }
+
+    /* TODO: We should also use TDR and RDR bits */
+
+    DPRINTF("End: TX Fifo Size = %d, RX Fifo Size = %d\n",
+            fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
+}
+
+static void imx_spi_reset(DeviceState *dev)
+{
+    IMXSPIState *s = IMX_SPI(dev);
+    int i;
+
+    DPRINTF("\n");
+
+    memset(s->regs, 0, ECSPI_MAX * sizeof(uint32_t));
+
+    s->regs[ECSPI_STATREG] = 0x00000003;
+
+    imx_spi_rxfifo_reset(s);
+    imx_spi_txfifo_reset(s);
+
+    imx_spi_update_irq(s);
+
+    s->burst_length = 0;
+
+    for (i=0; i<4; i++) {
+        qemu_set_irq(s->cs_lines[i], 0);
+    }
+}
+
+static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
+{
+    uint32 value = 0;
+    IMXSPIState *s = (IMXSPIState *)opaque;
+    uint32_t index = offset >> 2;
+
+    if (index >=  ECSPI_MAX) {
+        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
+                      HWADDR_PRIx "\n", TYPE_IMX_SPI, __func__, offset);
+        return 0;
+    }
+
+    switch (index) {
+    case ECSPI_RXDATA:
+        if (!imx_spi_is_enabled(s)) {
+            value = 0;
+        } else if (fifo32_is_empty(&s->rx_fifo)) {
+            value = 0xdeadbeef;
+        } else {
+            /* read from the RX FIFO */
+            value = fifo32_pop(&s->rx_fifo);
+        }
+
+        break;
+    case ECSPI_TXDATA:
+        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from TX FIFO\n",
+                      TYPE_IMX_SPI, __func__);
+
+        /* Reading from TXDATA gives 0 */
+
+        break;
+    case ECSPI_MSGDATA:
+        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from MSG FIFO\n",
+                      TYPE_IMX_SPI, __func__);
+
+        /* Reading from MSGDATA gives 0 */
+
+        break;
+    default:
+        value = s->regs[index];
+        break;
+    }
+
+    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
+
+    imx_spi_update_irq(s);
+
+    return (uint64_t)value;
+}
+
+static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
+                           unsigned size)
+{
+    IMXSPIState *s = (IMXSPIState *)opaque;
+    uint32_t index = offset >> 2;
+    uint32_t change_mask;
+
+    if (index >=  ECSPI_MAX) {
+        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
+                      HWADDR_PRIx "\n", TYPE_IMX_SPI, __func__, offset);
+        return;
+    }
+
+    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
+            (uint32_t)value);
+
+    change_mask = s->regs[index] ^ value;
+
+    switch (index) {
+    case ECSPI_RXDATA:
+        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to write to RX FIFO\n",
+                      TYPE_IMX_SPI, __func__);
+        break;
+    case ECSPI_TXDATA:
+    case ECSPI_MSGDATA:
+        /* Is there any difference between TXDATA and MSGDATA ? */
+        /* I'll have to look in the linux driver */
+        if (!imx_spi_is_enabled(s)) {
+            /* Ignore writes if device is disabled */
+            break;
+        } else if (fifo32_is_full(&s->tx_fifo)) {
+            /* Ignore writes if queue is full */
+            break;
+        }
+
+        fifo32_push(&s->tx_fifo, (uint32_t)value);
+
+        if (imx_spi_channel_is_master(s) && 
+            (s->regs[ECSPI_CONREG] & ECSPI_CONREG_SMC)) {
+            /*
+             * Start emiting if current channel is master and SMC bit is
+             * set.
+             */
+            imx_spi_flush_txfifo(s);
+        }
+
+        break;
+    case ECSPI_STATREG:
+        /* Clear RO and TC bits on write */
+        value &= ECSPI_STATREG_RO | ECSPI_STATREG_TC;
+        s->regs[ECSPI_STATREG] &= ~value;
+
+        break;
+    case ECSPI_CONREG:
+        s->regs[ECSPI_CONREG] = value;
+
+        if (!imx_spi_is_enabled(s)) {
+            /* device is diabled, so this is a reset */
+            imx_spi_reset(DEVICE(s));
+            return;
+        }
+
+        if (imx_spi_channel_is_master(s)) {
+            int i;
+
+            /* We are in master mode */
+
+            for (i=0; i<4; i++) {
+                qemu_set_irq(s->cs_lines[i],
+                             i == imx_spi_selected_channel(s) ? 0 : 1);
+            }
+
+            if ((value & change_mask & ECSPI_CONREG_SMC) &&
+                !fifo32_is_empty(&s->tx_fifo)) {
+                /* SMC bit is set and TX FIFO has some slots filled in */
+                imx_spi_flush_txfifo(s);
+            } else if ((value & change_mask & ECSPI_CONREG_XCH) &&
+                !(value & ECSPI_CONREG_SMC)) {
+                /* This is a request to start emiting */
+                imx_spi_flush_txfifo(s);
+            }
+        }
+
+        break;
+    default:
+        s->regs[index] = value;
+
+        break;
+    }
+
+    imx_spi_update_irq(s);
+}
+
+static const struct MemoryRegionOps imx_spi_ops = {
+    .read = imx_spi_read,
+    .write = imx_spi_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        /*
+         * Our device would not work correctly if the guest was doing
+         * unaligned access. This might not be a limitation on the real
+         * device but in practice there is no reason for a guest to access
+         * this device unaligned.
+         */
+        .min_access_size = 4,
+        .max_access_size = 4,
+        .unaligned = false,
+    },
+};
+
+static void imx_spi_realize(DeviceState *dev, Error **errp)
+{
+    IMXSPIState *s = IMX_SPI(dev);
+    int i;
+
+    s->bus = ssi_create_bus(dev, "spi");
+
+    memory_region_init_io(&s->iomem, OBJECT(dev), &imx_spi_ops, s,
+                          TYPE_IMX_SPI, 0x1000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
+    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
+
+    ssi_auto_connect_slaves(dev, s->cs_lines, s->bus);
+
+    for (i = 0; i < 4; ++i) {
+        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cs_lines[i]);
+    }
+
+    s->previous_level = -1;
+    s->burst_length = 0;
+
+    fifo32_create(&s->tx_fifo, ECSPI_FIFO_SIZE);
+    fifo32_create(&s->rx_fifo, ECSPI_FIFO_SIZE);
+}
+
+static void imx_spi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = imx_spi_realize;
+    dc->vmsd = &vmstate_imx_spi;
+    dc->reset = imx_spi_reset;
+    dc->desc = "i.MX SPI Controller";
+}
+
+static const TypeInfo imx_spi_info = {
+    .name          = TYPE_IMX_SPI,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IMXSPIState),
+    .class_init    = imx_spi_class_init,
+};
+
+static void imx_spi_register_types(void)
+{
+    type_register_static(&imx_spi_info);
+}
+
+type_init(imx_spi_register_types)
diff --git a/include/hw/ssi/imx_spi.h b/include/hw/ssi/imx_spi.h
new file mode 100644
index 0000000..1e38cb1
--- /dev/null
+++ b/include/hw/ssi/imx_spi.h
@@ -0,0 +1,104 @@
+/*
+ * IMX SPI Controller
+ *
+ * Copyright 2016 Jean-Christophe Dubois <jcd@tribudubois.net>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef IMX_SPI_H
+#define IMX_SPI_H
+
+#include "hw/sysbus.h"
+#include "hw/ssi/ssi.h"
+#include "qemu/bitops.h"
+#include "qemu/fifo32.h"
+
+#define ECSPI_FIFO_SIZE 64
+
+#define ECSPI_RXDATA 0
+#define ECSPI_TXDATA 1
+#define ECSPI_CONREG 2
+#define ECSPI_CONFIGREG 3
+#define ECSPI_INTREG 4
+#define ECSPI_DMAREG 5
+#define ECSPI_STATREG 6
+#define ECSPI_PERIODREG 7
+#define ECSPI_TESTREG 8
+#define ECSPI_MSGDATA 16
+#define ECSPI_MAX 17
+
+/* ECSPI_CONREG */
+#define ECSPI_CONREG_EN (1 << 0)
+#define ECSPI_CONREG_HT (1 << 1)
+#define ECSPI_CONREG_XCH (1 << 2)
+#define ECSPI_CONREG_SMC (1 << 3)
+#define ECSPI_CONREG_CHANNEL_MODE_SHIFT 4
+#define ECSPI_CONREG_CHANNEL_MODE_LENGTH 4
+#define ECSPI_CONREG_DRCTL_SHIFT 16
+#define ECSPI_CONREG_DRCTL_LENGTH 2
+#define ECSPI_CONREG_CHANNEL_SELECT_SHIFT 18
+#define ECSPI_CONREG_CHANNEL_SELECT_LENGTH 2
+#define ECSPI_CONREG_BURST_LENGTH_SHIFT 20
+#define ECSPI_CONREG_BURST_LENGTH_LENGTH 12
+
+/* ECSPI_CONFIGREG */
+#define ECSPI_CONFIGREG_SS_CTL_SHIFT 8
+#define ECSPI_CONFIGREG_SS_CTL_LENGTH 4
+
+/* ECSPI_INTREG */
+#define ECSPI_INTREG_TEEN (1 << 0)
+#define ECSPI_INTREG_TDREN (1 << 1)
+#define ECSPI_INTREG_TFEN (1 << 2)
+#define ECSPI_INTREG_RREN (1 << 3)
+#define ECSPI_INTREG_RDREN (1 << 4)
+#define ECSPI_INTREG_RFEN (1 << 5)
+#define ECSPI_INTREG_ROEN (1 << 6)
+#define ECSPI_INTREG_TCEN (1 << 7)
+
+/* ECSPI_DMAREG */
+#define ECSPI_DMAREG_RXTDEN (1 << 31)
+#define ECSPI_DMAREG_RXDEN (1 << 23)
+#define ECSPI_DMAREG_TEDEN (1 << 7)
+#define ECSPI_DMAREG_RX_THRESHOLD_SHIFT 16
+#define ECSPI_DMAREG_RX_THRESHOLD_LENGTH 6
+
+/* ECSPI_STATREG */
+#define ECSPI_STATREG_TE (1 << 0)
+#define ECSPI_STATREG_TDR (1 << 1)
+#define ECSPI_STATREG_TF (1 << 2)
+#define ECSPI_STATREG_RR (1 << 3)
+#define ECSPI_STATREG_RDR (1 << 4)
+#define ECSPI_STATREG_RF (1 << 5)
+#define ECSPI_STATREG_RO (1 << 6)
+#define ECSPI_STATREG_TC (1 << 7)
+
+#define EXTRACT(value, name) extract32(value, name##_SHIFT, name##_LENGTH)
+
+#define TYPE_IMX_SPI "imx.spi"
+#define IMX_SPI(obj) OBJECT_CHECK(IMXSPIState, (obj), TYPE_IMX_SPI)
+
+typedef struct IMXSPIState {
+    /* <private> */
+    SysBusDevice parent_obj;
+
+    /* <public> */
+    MemoryRegion iomem;
+
+    qemu_irq irq;
+    int previous_level;
+
+    qemu_irq cs_lines[4];
+
+    SSIBus *bus;
+
+    uint32_t regs[ECSPI_MAX];
+
+    Fifo32 rx_fifo;
+    Fifo32 tx_fifo;
+
+    int16_t burst_length;
+} IMXSPIState;
+
+#endif /* IMX_SPI_H */
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 2/4] i.MX: Add the Freescale SPI Controller

[Peter Maydell]

On 15 February 2016 at 11:17, Jean-Christophe Dubois
<jcd@tribudubois.net> wrote:
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
>
> Changes since v1:
>  * Access SPI slave only at a byte level.
>  * rework the CS activation to avoid to reset access to SPI slaves.
>
>  hw/ssi/Makefile.objs     |   1 +
>  hw/ssi/imx_spi.c         | 459 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ssi/imx_spi.h | 104 +++++++++++
>  3 files changed, 564 insertions(+)
>  create mode 100644 hw/ssi/imx_spi.c
>  create mode 100644 include/hw/ssi/imx_spi.h
>
> diff --git a/hw/ssi/Makefile.objs b/hw/ssi/Makefile.objs
> index 9555825..fcbb79e 100644
> --- a/hw/ssi/Makefile.objs
> +++ b/hw/ssi/Makefile.objs
> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
>  common-obj-$(CONFIG_XILINX_SPIPS) += xilinx_spips.o
>
>  obj-$(CONFIG_OMAP) += omap_spi.o
> +obj-$(CONFIG_IMX) += imx_spi.o
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> new file mode 100644
> index 0000000..cab102c
> --- /dev/null
> +++ b/hw/ssi/imx_spi.c
> @@ -0,0 +1,459 @@
> +/*
> + * IMX SPI Controller
> + *
> + * Copyright (c) 2016 Jean-Christophe Dubois <jcd@tribudubois.net>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hw/ssi/imx_spi.h"
> +#include "sysemu/sysemu.h"

Source files need to #include "qemu/osdep.h" as the first #include.
(You will find this doesn't compile otherwise if you rebase on master.)

> +
> +#ifndef DEBUG_IMX_SPI
> +#define DEBUG_IMX_SPI 0
> +#endif
> +
> +#define DPRINTF(fmt, args...) \
> +    do { \
> +        if (DEBUG_IMX_SPI) { \
> +            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SPI, \
> +                                             __func__, ##args); \
> +        } \
> +    } while (0)
> +
> +static char const *imx_spi_reg_name(uint32_t reg)
> +{
> +    static char unknown[20];
> +
> +    switch (reg) {
> +    case ECSPI_RXDATA:
> +        return  "ECSPI_RXDATA";
> +    case ECSPI_TXDATA:
> +        return  "ECSPI_TXDATA";
> +    case ECSPI_CONREG:
> +        return  "ECSPI_CONREG";
> +    case ECSPI_CONFIGREG:
> +        return  "ECSPI_CONFIGREG";
> +    case ECSPI_INTREG:
> +        return  "ECSPI_INTREG";
> +    case ECSPI_DMAREG:
> +        return  "ECSPI_DMAREG";
> +    case ECSPI_STATREG:
> +        return  "ECSPI_STATREG";
> +    case ECSPI_PERIODREG:
> +        return  "ECSPI_PERIODREG";
> +    case ECSPI_TESTREG:
> +        return  "ECSPI_TESTREG";
> +    case ECSPI_MSGDATA:
> +        return  "ECSPI_MSGDATA";
> +    default:
> +        sprintf(unknown, "%d ?", reg);
> +        return unknown;
> +    }
> +}
> +
> +static const VMStateDescription vmstate_imx_spi = {
> +    .name = TYPE_IMX_SPI,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_FIFO32(tx_fifo, IMXSPIState),
> +        VMSTATE_FIFO32(rx_fifo, IMXSPIState),
> +        VMSTATE_UINT32_ARRAY(regs, IMXSPIState, ECSPI_MAX),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static void imx_spi_txfifo_reset(IMXSPIState *s)
> +{
> +    fifo32_reset(&s->tx_fifo);
> +    s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
> +    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
> +}
> +
> +static void imx_spi_rxfifo_reset(IMXSPIState *s)
> +{
> +    fifo32_reset(&s->rx_fifo);
> +    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
> +    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
> +    s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RO;
> +}
> +
> +static void imx_spi_update_irq(IMXSPIState *s)
> +{
> +    int level;
> +
> +    if (fifo32_is_empty(&s->rx_fifo)) {
> +        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
> +    } else {
> +        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RR;
> +    }
> +
> +    if (fifo32_is_full(&s->rx_fifo)) {
> +        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RF;
> +    } else {
> +        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
> +    }
> +
> +    if (fifo32_is_empty(&s->tx_fifo)) {
> +        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
> +    } else {
> +        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TE;
> +    }
> +
> +    if (fifo32_is_full(&s->tx_fifo)) {
> +        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TF;
> +    } else {
> +        s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
> +    }
> +
> +    level = s->regs[ECSPI_STATREG] & s->regs[ECSPI_INTREG] ? 1 : 0;
> +
> +    if (s->previous_level != level) {
> +        DPRINTF("setting IRQ a level %d\n", level);
> +        s->previous_level = level;
> +        qemu_set_irq(s->irq, level);
> +    }

s->previous_level isn't real hardware device state, and it isn't
in your migration struct either. You can either:
(a) just go ahead and always call qemu_set_irq() even if maybe
the level didn't change
(b) calculate previous_level before you change any of the
STATREG or INTREG bits

(a) is simplest.

> +    DPRINTF("IRQ level is %d\n", level);
> +}
> +
> +static uint8_t imx_spi_selected_channel(IMXSPIState *s)
> +{
> +    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_CHANNEL_SELECT);
> +}
> +
> +static uint32_t imx_spi_burst_length(IMXSPIState *s)
> +{
> +    return EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_BURST_LENGTH) + 1;
> +}
> +
> +static bool imx_spi_is_enabled(IMXSPIState *s)
> +{
> +    return (s->regs[ECSPI_CONREG] & ECSPI_CONREG_EN) ? true : false;

 "X ? true : false" as a bool is just "X".

> +}
> +
> +static bool imx_spi_channel_is_master(IMXSPIState *s)
> +{
> +    uint8_t mode = EXTRACT(s->regs[ECSPI_CONREG], ECSPI_CONREG_CHANNEL_MODE);
> +
> +    return (mode & (1 << imx_spi_selected_channel(s))) ? true : false;
> +}
> +
> +static bool imx_spi_is_multiple_master_burst(IMXSPIState *s)
> +{
> +    uint8_t wave = EXTRACT(s->regs[ECSPI_CONFIGREG], ECSPI_CONFIGREG_SS_CTL);
> +
> +    return imx_spi_channel_is_master(s) &&
> +           !(s->regs[ECSPI_CONREG] & ECSPI_CONREG_SMC) &&
> +           ((wave & (1 << imx_spi_selected_channel(s))) ? true : false);
> +}
> +
> +static void imx_spi_flush_txfifo(IMXSPIState *s)
> +{
> +    uint32_t tx;
> +    uint32_t rx;
> +
> +    DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n",
> +            fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
> +
> +    while (!fifo32_is_empty(&s->tx_fifo)) {
> +        int tx_burst = 0;
> +
> +        if (s->burst_length <= 0) {
> +            s->burst_length = imx_spi_burst_length(s);

Why is s->burst_length in your state struct? It isn't in your
migration state structure. Does it really correspond to
hardware state that isn't the same as the register field?

> +
> +            DPRINTF("Burst length = %d\n", s->burst_length);
> +
> +            if (imx_spi_is_multiple_master_burst(s)) {
> +                s->regs[ECSPI_CONREG] |= ECSPI_CONREG_XCH;
> +            }
> +        }
> +
> +        tx = fifo32_pop(&s->tx_fifo);
> +
> +        DPRINTF("data tx:0x%08x\n", tx);
> +
> +        tx_burst = MIN(s->burst_length, 32);
> +
> +        rx = 0;
> +
> +        while (tx_burst) {
> +            rx = rx << 8;
> +
> +            DPRINTF("writing 0x%02x\n", tx & 0xff);
> +
> +            /* We need to write one byte at a time */
> +            rx |= ssi_transfer(s->bus, tx & 0xFF) & 0xFF;
> +
> +            DPRINTF("0x%02x read\n", rx & 0xff);

Consistency about whether you write hex as 0xFF or 0xff would be nice.

> +
> +            tx = tx >> 8;
> +
> +            /* Remove 8 bits from the actual burst */
> +            tx_burst -= 8;
> +
> +            s->burst_length -= 8;
> +        }
> +
> +        DPRINTF("data rx:0x%08x\n", rx);
> +
> +        if (fifo32_is_full(&s->rx_fifo)) {
> +            s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
> +        } else {
> +            fifo32_push(&s->rx_fifo, (uint8_t)rx);
> +        }
> +
> +        if (s->burst_length <= 0) {
> +            s->regs[ECSPI_CONREG] &= ~ECSPI_CONREG_XCH;
> +
> +            if (!imx_spi_is_multiple_master_burst(s)) {
> +                s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
> +                break;
> +            }
> +        }
> +    }
> +
> +    if (fifo32_is_empty(&s->tx_fifo)) {
> +        s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TC;
> +    }
> +
> +    /* TODO: We should also use TDR and RDR bits */
> +
> +    DPRINTF("End: TX Fifo Size = %d, RX Fifo Size = %d\n",
> +            fifo32_num_used(&s->tx_fifo), fifo32_num_used(&s->rx_fifo));
> +}
> +
> +static void imx_spi_reset(DeviceState *dev)
> +{
> +    IMXSPIState *s = IMX_SPI(dev);
> +    int i;
> +
> +    DPRINTF("\n");
> +
> +    memset(s->regs, 0, ECSPI_MAX * sizeof(uint32_t));

You could just write sizeof(s->regs).

> +
> +    s->regs[ECSPI_STATREG] = 0x00000003;
> +
> +    imx_spi_rxfifo_reset(s);
> +    imx_spi_txfifo_reset(s);
> +
> +    imx_spi_update_irq(s);

Updating outbound IRQs in a reset function is better avoided
(though this area of QEMU is a mess).

> +
> +    s->burst_length = 0;
> +
> +    for (i=0; i<4; i++) {
> +        qemu_set_irq(s->cs_lines[i], 0);
> +    }
> +}
> +
> +static uint64_t imx_spi_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    uint32 value = 0;
> +    IMXSPIState *s = (IMXSPIState *)opaque;

You don't need to explicitly cast void pointers.

> +    uint32_t index = offset >> 2;
> +
> +    if (index >=  ECSPI_MAX) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> +                      HWADDR_PRIx "\n", TYPE_IMX_SPI, __func__, offset);
> +        return 0;
> +    }
> +
> +    switch (index) {
> +    case ECSPI_RXDATA:
> +        if (!imx_spi_is_enabled(s)) {
> +            value = 0;
> +        } else if (fifo32_is_empty(&s->rx_fifo)) {
> +            value = 0xdeadbeef;

Is this really what the hardware does? If the h/w spec says "undefined value"
or something then it's worth a comment to that effect.

> +        } else {
> +            /* read from the RX FIFO */
> +            value = fifo32_pop(&s->rx_fifo);
> +        }
> +
> +        break;
> +    case ECSPI_TXDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from TX FIFO\n",
> +                      TYPE_IMX_SPI, __func__);
> +
> +        /* Reading from TXDATA gives 0 */
> +
> +        break;
> +    case ECSPI_MSGDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read from MSG FIFO\n",
> +                      TYPE_IMX_SPI, __func__);
> +
> +        /* Reading from MSGDATA gives 0 */
> +
> +        break;
> +    default:
> +        value = s->regs[index];
> +        break;
> +    }
> +
> +    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_spi_reg_name(index), value);
> +
> +    imx_spi_update_irq(s);
> +
> +    return (uint64_t)value;
> +}
> +
> +static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    IMXSPIState *s = (IMXSPIState *)opaque;
> +    uint32_t index = offset >> 2;
> +    uint32_t change_mask;
> +
> +    if (index >=  ECSPI_MAX) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> +                      HWADDR_PRIx "\n", TYPE_IMX_SPI, __func__, offset);
> +        return;
> +    }
> +
> +    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_spi_reg_name(index),
> +            (uint32_t)value);
> +
> +    change_mask = s->regs[index] ^ value;
> +
> +    switch (index) {
> +    case ECSPI_RXDATA:
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to write to RX FIFO\n",
> +                      TYPE_IMX_SPI, __func__);
> +        break;
> +    case ECSPI_TXDATA:
> +    case ECSPI_MSGDATA:
> +        /* Is there any difference between TXDATA and MSGDATA ? */
> +        /* I'll have to look in the linux driver */

That sounds like it would be a good plan :-)

> +        if (!imx_spi_is_enabled(s)) {
> +            /* Ignore writes if device is disabled */
> +            break;
> +        } else if (fifo32_is_full(&s->tx_fifo)) {
> +            /* Ignore writes if queue is full */
> +            break;
> +        }
> +
> +        fifo32_push(&s->tx_fifo, (uint32_t)value);
> +
> +        if (imx_spi_channel_is_master(s) &&
> +            (s->regs[ECSPI_CONREG] & ECSPI_CONREG_SMC)) {
> +            /*
> +             * Start emiting if current channel is master and SMC bit is

"emitting"

> +             * set.
> +             */
> +            imx_spi_flush_txfifo(s);
> +        }
> +
> +        break;
> +    case ECSPI_STATREG:
> +        /* Clear RO and TC bits on write */

Do you mean "the RO and TC bits are write-one-to-clear" ?

> +        value &= ECSPI_STATREG_RO | ECSPI_STATREG_TC;
> +        s->regs[ECSPI_STATREG] &= ~value;
> +
> +        break;
> +    case ECSPI_CONREG:
> +        s->regs[ECSPI_CONREG] = value;
> +
> +        if (!imx_spi_is_enabled(s)) {
> +            /* device is diabled, so this is a reset */

"disabled"

> +            imx_spi_reset(DEVICE(s));
> +            return;
> +        }
> +
> +        if (imx_spi_channel_is_master(s)) {
> +            int i;
> +
> +            /* We are in master mode */
> +
> +            for (i=0; i<4; i++) {
> +                qemu_set_irq(s->cs_lines[i],
> +                             i == imx_spi_selected_channel(s) ? 0 : 1);
> +            }
> +
> +            if ((value & change_mask & ECSPI_CONREG_SMC) &&
> +                !fifo32_is_empty(&s->tx_fifo)) {
> +                /* SMC bit is set and TX FIFO has some slots filled in */
> +                imx_spi_flush_txfifo(s);
> +            } else if ((value & change_mask & ECSPI_CONREG_XCH) &&
> +                !(value & ECSPI_CONREG_SMC)) {
> +                /* This is a request to start emiting */

"emitting"

> +                imx_spi_flush_txfifo(s);
> +            }
> +        }
> +
> +        break;
> +    default:
> +        s->regs[index] = value;
> +
> +        break;
> +    }
> +
> +    imx_spi_update_irq(s);
> +}

thanks
-- PMM

[Qemu-devel] [PATCH v2 3/4] i.MX: Add SPI controllers to i.MX6 SOC

[Jean-Christophe Dubois]

This allows Linux to boot without hanging on SPI access.

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---

Changes since V1:
 * None

 hw/arm/fsl-imx6.c         | 32 ++++++++++++++++++++++++++++++++
 include/hw/arm/fsl-imx6.h |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 0faae27..c5c8fdc 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -78,6 +78,11 @@ static void fsl_imx6_init(Object *obj)
         object_initialize(&s->esdhc[i], sizeof(s->esdhc[i]), TYPE_SYSBUS_SDHCI);
         qdev_set_parent_bus(DEVICE(&s->esdhc[i]), sysbus_get_default());
     }
+
+    for (i = 0; i < FSL_IMX6_NUM_ECSPIS; i++) {
+        object_initialize(&s->spi[i], sizeof(s->spi[i]), TYPE_IMX_SPI);
+        qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
+    }
 }
 
 static void fsl_imx6_realize(DeviceState *dev, Error **errp)
@@ -339,6 +344,33 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                             esdhc_table[i].irq));
     }
 
+    /* Initialize all ECSPI */
+    for (i = 0; i < FSL_IMX6_NUM_ECSPIS; i++) {
+        static const struct {
+            hwaddr addr;
+            unsigned int irq;
+        } spi_table[FSL_IMX6_NUM_ECSPIS] = {
+            { FSL_IMX6_eCSPI1_ADDR, FSL_IMX6_ECSPI1_IRQ },
+            { FSL_IMX6_eCSPI2_ADDR, FSL_IMX6_ECSPI2_IRQ },
+            { FSL_IMX6_eCSPI3_ADDR, FSL_IMX6_ECSPI3_IRQ },
+            { FSL_IMX6_eCSPI4_ADDR, FSL_IMX6_ECSPI4_IRQ },
+            { FSL_IMX6_eCSPI5_ADDR, FSL_IMX6_ECSPI5_IRQ },
+        };
+
+        /* Initialize the SPI */
+        object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+        /* Map SPI memory */
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0, spi_table[i].addr);
+        /* Connect SPI IRQ to PIC */
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->spi[i]), 0,
+                           qdev_get_gpio_in(DEVICE(&s->a9mpcore),
+                                            spi_table[i].irq));
+    }
+
     /* ROM memory */
     memory_region_init_rom_device(&s->rom, NULL, NULL, NULL, "imx6.rom",
                                   FSL_IMX6_ROM_SIZE, &err);
diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 301812d..d24aaee 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -27,6 +27,7 @@
 #include "hw/i2c/imx_i2c.h"
 #include "hw/gpio/imx_gpio.h"
 #include "hw/sd/sdhci.h"
+#include "hw/ssi/imx_spi.h"
 #include "exec/memory.h"
 
 #define TYPE_FSL_IMX6 "fsl,imx6"
@@ -38,6 +39,7 @@
 #define FSL_IMX6_NUM_I2CS 3
 #define FSL_IMX6_NUM_GPIOS 7
 #define FSL_IMX6_NUM_ESDHCS 4
+#define FSL_IMX6_NUM_ECSPIS 5
 
 typedef struct FslIMX6State {
     /*< private >*/
@@ -54,6 +56,7 @@ typedef struct FslIMX6State {
     IMXI2CState    i2c[FSL_IMX6_NUM_I2CS];
     IMXGPIOState   gpio[FSL_IMX6_NUM_GPIOS];
     SDHCIState     esdhc[FSL_IMX6_NUM_ESDHCS];
+    IMXSPIState    spi[FSL_IMX6_NUM_ECSPIS];
     MemoryRegion   rom;
     MemoryRegion   caam;
     MemoryRegion   ocram;
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 3/4] i.MX: Add SPI controllers to i.MX6 SOC

[Peter Maydell]

On 15 February 2016 at 11:18, Jean-Christophe Dubois
<jcd@tribudubois.net> wrote:
> This allows Linux to boot without hanging on SPI access.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v2 4/4] i.MX: Add SPI NOR FLASH memory to sabrelite board.

[Jean-Christophe Dubois]

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---

Changes since v1:
 * Not present on v1.

 hw/arm/sabrelite.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index 8db9bbc..237dfa1 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -70,6 +70,15 @@ static void sabrelite_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
                                 &s->ram);
 
+    {
+        /* Add the sst25vf016b NOR FLASH memory to first SPI */
+        SSIBus *spi = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc.spi[0]),
+                                                   "spi");
+        DeviceState *flash_dev = ssi_create_slave(spi, "sst25vf016b");
+        qemu_irq cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[0]), 1, cs_line);
+    }
+
     sabrelite_binfo.ram_size = machine->ram_size;
     sabrelite_binfo.kernel_filename = machine->kernel_filename;
     sabrelite_binfo.kernel_cmdline = machine->kernel_cmdline;
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 4/4] i.MX: Add SPI NOR FLASH memory to sabrelite board.

[Peter Maydell]

On 15 February 2016 at 11:18, Jean-Christophe Dubois
<jcd@tribudubois.net> wrote:
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
>
> Changes since v1:
>  * Not present on v1.
>
>  hw/arm/sabrelite.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index 8db9bbc..237dfa1 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -70,6 +70,15 @@ static void sabrelite_init(MachineState *machine)
>      memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
>                                  &s->ram);
>
> +    {
> +        /* Add the sst25vf016b NOR FLASH memory to first SPI */
> +        SSIBus *spi = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc.spi[0]),
> +                                                   "spi");

Rather than having the board code looking into the internals
of the SoC struct like this, you should have the SoC create an
spi bus property for itself, and then here you can just have

      spibus = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc), "spi");

See hw/arm/xlnx-ep108.c for an example of this.
The code to create the property in the SoC's realize method is

        object_property_add_alias(OBJECT(s), "spi",
                                  OBJECT(&s->spi[i]), "spi",
                                  &error_abort);

(where the first "spi" is the name of the bus property to create on the
SoC object, and the second is the name of the bus property on the
internal SPI device object). Example code in hw/arm/xlnx-zynqmp.c.

> +        DeviceState *flash_dev = ssi_create_slave(spi, "sst25vf016b");
> +        qemu_irq cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[0]), 1, cs_line);

Again, you want the SoC to provide an outward-facing interface to the
chipselect line so you don't have to have the board code looking
into soc.spi[] itself.

> +    }
> +
>      sabrelite_binfo.ram_size = machine->ram_size;
>      sabrelite_binfo.kernel_filename = machine->kernel_filename;
>      sabrelite_binfo.kernel_cmdline = machine->kernel_cmdline;

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] i.MX: Add SPI NOR FLASH memory to sabrelite board.

[Jean-Christophe DUBOIS]

Le 25/02/2016 15:33, Peter Maydell a écrit :
> On 15 February 2016 at 11:18, Jean-Christophe Dubois
> <jcd@tribudubois.net> wrote:
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> ---
>>
>> Changes since v1:
>>   * Not present on v1.
>>
>>   hw/arm/sabrelite.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
>> index 8db9bbc..237dfa1 100644
>> --- a/hw/arm/sabrelite.c
>> +++ b/hw/arm/sabrelite.c
>> @@ -70,6 +70,15 @@ static void sabrelite_init(MachineState *machine)
>>       memory_region_add_subregion(get_system_memory(), FSL_IMX6_MMDC_ADDR,
>>                                   &s->ram);
>>
>> +    {
>> +        /* Add the sst25vf016b NOR FLASH memory to first SPI */
>> +        SSIBus *spi = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc.spi[0]),
>> +                                                   "spi");
> Rather than having the board code looking into the internals
> of the SoC struct like this, you should have the SoC create an
> spi bus property for itself, and then here you can just have
>
>        spibus = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc), "spi");

I have 5 SPI Controllers so I should do it for each of them. And maybe 
also for I2C, SDHC  and other devices ...

There might be a lot of properties a the end ...

What would be the rule?

Is there a recommended naming scheme?

>
> See hw/arm/xlnx-ep108.c for an example of this.
> The code to create the property in the SoC's realize method is
>
>          object_property_add_alias(OBJECT(s), "spi",
>                                    OBJECT(&s->spi[i]), "spi",
>                                    &error_abort);
>
> (where the first "spi" is the name of the bus property to create on the
> SoC object, and the second is the name of the bus property on the
> internal SPI device object). Example code in hw/arm/xlnx-zynqmp.c.

OK, I'll have a look.
>
>> +        DeviceState *flash_dev = ssi_create_slave(spi, "sst25vf016b");
>> +        qemu_irq cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[0]), 1, cs_line);
> Again, you want the SoC to provide an outward-facing interface to the
> chipselect line so you don't have to have the board code looking
> into soc.spi[] itself.

OK

>
>> +    }
>> +
>>       sabrelite_binfo.ram_size = machine->ram_size;
>>       sabrelite_binfo.kernel_filename = machine->kernel_filename;
>>       sabrelite_binfo.kernel_cmdline = machine->kernel_cmdline;
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v2 4/4] i.MX: Add SPI NOR FLASH memory to sabrelite board.

[Peter Maydell]

On 25 February 2016 at 20:37, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> Le 25/02/2016 15:33, Peter Maydell a écrit :
>>
>> On 15 February 2016 at 11:18, Jean-Christophe Dubois
>> <jcd@tribudubois.net> wrote:
>>>
>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>> ---
>>>
>>> Changes since v1:
>>>   * Not present on v1.
>>>
>>>   hw/arm/sabrelite.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
>>> index 8db9bbc..237dfa1 100644
>>> --- a/hw/arm/sabrelite.c
>>> +++ b/hw/arm/sabrelite.c
>>> @@ -70,6 +70,15 @@ static void sabrelite_init(MachineState *machine)
>>>       memory_region_add_subregion(get_system_memory(),
>>> FSL_IMX6_MMDC_ADDR,
>>>                                   &s->ram);
>>>
>>> +    {
>>> +        /* Add the sst25vf016b NOR FLASH memory to first SPI */
>>> +        SSIBus *spi = (SSIBus
>>> *)qdev_get_child_bus(DEVICE(&s->soc.spi[0]),
>>> +                                                   "spi");
>>
>> Rather than having the board code looking into the internals
>> of the SoC struct like this, you should have the SoC create an
>> spi bus property for itself, and then here you can just have
>>
>>        spibus = (SSIBus *)qdev_get_child_bus(DEVICE(&s->soc), "spi");
>
>
> I have 5 SPI Controllers so I should do it for each of them. And maybe also
> for I2C, SDHC  and other devices ...
>
> There might be a lot of properties a the end ...
>
> What would be the rule?
>
> Is there a recommended naming scheme?

We seem to be using "spi0", "spi1", etc. The zynqmp actually
has multiple SPI controllers too so you can use the code it
has to loop round creating properties.

thanks
-- PMM