[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v6] hw/ssi/imx_spi.c: fix CS handling during SPI

From: Jean-Christophe DUBOIS
Subject: Re: [Qemu-devel] [PATCH v6] hw/ssi/imx_spi.c: fix CS handling during SPI access.
Date: Thu, 19 Jan 2017 22:15:50 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

Le 16/01/2017 à 20:06, mar.krzeminski a écrit :
W dniu 16.01.2017 o 18:22, Peter Maydell pisze:
On 11 January 2017 at 20:00, Jean-Christophe Dubois <address@hidden> wrote:
The i.MX SPI device was not de-asserting the CS line at the end of
memory access.

This triggered a SIGSEGV in Qemu when the sabrelite emulator was acessing
a SPI flash memory.

Whith this path the CS signal is correctly asserted and deasserted arround
memory access.

Assertion level is now based on SPI device configuration.

This was tested by:
* booting linux on Sabrelite Qemu emulator.
* booting xvisor on Sabrelite Qemu emultor.

Signed-off-by: Jean-Christophe Dubois <address@hidden>
Acked-by: Marcin Krzemiński <address@hidden>
I think it could be nice to split this in two patches - one regarding CS line and one about multi master mode.
My Acked-By apply to the not handled CS line in model only.

But these 2 functions are tied together. The CS handling is dependent to the multi master burst. The CS signal need to be active during the all multi master burst.
hw/ssi/imx_spi.c | 148 ++++++++++++++++++++++++++++++++++++-----------
  include/hw/ssi/imx_spi.h |   4 ++
  2 files changed, 117 insertions(+), 35 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index e4e395f..d54c145 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -56,19 +56,6 @@ static const char *imx_spi_reg_name(uint32_t reg)

-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_INT16(burst_length, IMXSPIState),
-    },
  static void imx_spi_txfifo_reset(IMXSPIState *s)
@@ -119,6 +106,32 @@ static void imx_spi_update_irq(IMXSPIState *s)
      DPRINTF("IRQ level is %d\n", level);

+static int imx_spi_post_load(void *opaque, int version_id)
+    IMXSPIState *s = (IMXSPIState *)opaque;
+    imx_spi_update_irq(s);
+    /* TODO: We should also restore the CS line level */
I don't think you should need to do anything here -- if
this device's state has been restored and the state at
the other end of the line has been restored, there's nothing
else to do. It's like reset, you don't want to be asserting
irq lines in post_load.

Ok, I'll remove the comment.

+    if (imx_spi_is_multiple_master_burst(s)) {
+        /*
+         * If we are in multi master burst mode we need to call this
+         * function at least 2 times before deselecting the CS line.
+         * In practice the transfert ends (and the CS is deselected)
+         * when the guest write 0 to the INT reg (according to linux
+         * driver code).
+         */
+        s->txfifo_burst_count++;
+    }
@@ -380,6 +433,30 @@ static void imx_spi_write(void *opaque, hwaddr offset, uint64_t value,

+    case ECSPI_INTREG:
+        s->regs[index] = value;
+        /* Disable CS lines */
+        if ((value == 0) && (s->txfifo_burst_count > 1)) {
+            /*
+             * When 0 is writen to the INT register (disabling all
+             * interrupts) we need to deselect the device CS line.
+             * But only if the txfifo function has been called at
+             * least twice.
+             * Note: This seems a bit hacky and I would preffer to
+             * rely on something else to deselect the CS line. But it
+             * works correctly with the linux driver.
+             */
I am not sure, but this hack is because Linux driver on real HW push data to FIFO when
transmission is ongoing?

Well in fact not really. It is waiting for the TE interrupt handler (TXFIFO is empty so there is not really anymore data in flight) and in this handler it will empty the RXFIFO and refill the TXFIFO if there is more to write/read. When there is no more data to write to the TXFIFO, it will disable the interrupts (write 0 to INTREG). This is when (on only then) that the CS line needs to be deselected.

For example the driver might do a 4096 bytes read (64 * 64 bytes) on one burst (only one read instruction + address at the beginning).

+            /* Deactivate the current CS line */
+ qemu_set_irq(s->cs_lines[imx_spi_selected_channel(s)],
+                         !imx_spi_current_channel_pol(s));
+            /* reset the txfifo count to 0 */
+            s->txfifo_burst_count = 0;
+        }
I think this is a bad idea. We have one or two devices which have
similar kinds of hacky code in them that knows what the Linux
driver does. The problem is then twofold:
  (1) if the Linux driver changes then the hacks break
  (2) it's almost impossible to remove the hacks later, because
      the reasoning behind them (which guest kernels are affected,
      etc) gets lost and you don't know whether you're going to
      break previously-working setups if you try to remove the
      hack code

I think we really should try to get this right from the start
(ie "behave like the hardware does").
Totally agree, I was also doing similar hacks at the beginning and it went exactly like
Peter said. Since every Linux driver update I needed to fix Qemu models.

Well, for now I have not found a better way to handle the CS signals correctly.


-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]