[Qemu-devel] [PATCH arm-devs v1 00/15] Xilinx SPIPS fixes round 2

[Qemu-devel] [PATCH arm-devs v1 00/15] Xilinx SPIPS fixes round 2

[Peter Crosthwaite]

Updates to the Zynq SPI controller. Some QOMifying cleanup, followed by
a number of bugs/incompletnesses found by some (very) rigourous test
vectors.


Peter Crosthwaite (15):
  xilinx_spips: seperate SPI and QSPI as two classes
  xilinx_spips: Make interrupts clear on read
  xilinx_spips: Inhibit interrupts in LQSPI mode
  xilinx_spips: Add verbose LQSPI debug output
  xilinx_spips: lqspi: Dont trash config register
  xilinx_spips: Fix QSPI FIFO size
  xilinx_spips: Trash LQ page cache on mode change
  xilinx_spips: Add automatic start support
  xilinx_spips: Implement automatic CS
  xilinx_spips: Fix CTRL register RW bits
  xilinx_spips: Fix striping behaviour
  xilinx_spips: Debug msgs for Snoop state
  xilinx_spips: Multiple debug verbosity levels
  xilinx_spips: lqspi: Push more data to tx-fifo
  xilinx_spips: lqspi: Fix byte/misaligned access

 hw/arm/xilinx_zynq.c |    2 +-
 hw/xilinx_spips.c    |  265 +++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 200 insertions(+), 67 deletions(-)

[Qemu-devel] [PATCH arm-devs v1 01/15] xilinx_spips: seperate SPI and QSPI as two classes

[Peter Crosthwaite]

Make SPI and QSPI different classes. QSPIPS is setup as a child of SPIPS.
Only QSPI has the LQSPI functionality, so move all that to the child class.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed from v2:
User parent_obj as appropriate for QOM parents (PMM review)
Changed from v1:
Fixed compile bug (s/XILINX_SPIPS/XILINX_QSPIPS on QOM cast)

 hw/arm/xilinx_zynq.c |    2 +-
 hw/xilinx_spips.c    |   69 +++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 6f36286..db712b1 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -66,7 +66,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
     int num_busses =  is_qspi ? NUM_QSPI_BUSSES : 1;
     int num_ss = is_qspi ? NUM_QSPI_FLASHES : NUM_SPI_FLASHES;
 
-    dev = qdev_create(NULL, "xilinx,spips");
+    dev = qdev_create(NULL, is_qspi ? "xlnx.ps7-qspi" : "xlnx.ps7-spi");
     qdev_prop_set_uint8(dev, "num-txrx-bytes", is_qspi ? 4 : 1);
     qdev_prop_set_uint8(dev, "num-ss-bits", num_ss);
     qdev_prop_set_uint8(dev, "num-busses", num_busses);
diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index b2397f4..734adf0 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -129,7 +129,8 @@ typedef enum {
 } FlashCMD;
 
 typedef struct {
-    SysBusDevice busdev;
+    SysBusDevice parent_obj;
+
     MemoryRegion iomem;
     MemoryRegion mmlqspi;
 
@@ -149,15 +150,23 @@ typedef struct {
     uint8_t num_txrx_bytes;
 
     uint32_t regs[R_MAX];
+} XilinxSPIPS;
+
+typedef struct {
+    XilinxSPIPS parent_obj;
 
     uint32_t lqspi_buf[LQSPI_CACHE_SIZE];
     hwaddr lqspi_cached_addr;
-} XilinxSPIPS;
+} XilinxQSPIPS;
+
 
-#define TYPE_XILINX_SPIPS "xilinx,spips"
+#define TYPE_XILINX_SPIPS "xlnx.ps7-spi"
+#define TYPE_XILINX_QSPIPS "xlnx.ps7-qspi"
 
 #define XILINX_SPIPS(obj) \
      OBJECT_CHECK(XilinxSPIPS, (obj), TYPE_XILINX_SPIPS)
+#define XILINX_QSPIPS(obj) \
+     OBJECT_CHECK(XilinxQSPIPS, (obj), TYPE_XILINX_QSPIPS)
 
 static inline int num_effective_busses(XilinxSPIPS *s)
 {
@@ -436,11 +445,12 @@ static uint64_t
 lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 {
     int i;
+    XilinxQSPIPS *q = opaque;
     XilinxSPIPS *s = opaque;
 
-    if (addr >= s->lqspi_cached_addr &&
-            addr <= s->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
-        return s->lqspi_buf[(addr - s->lqspi_cached_addr) >> 2];
+    if (addr >= q->lqspi_cached_addr &&
+            addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
+        return q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
     } else {
         int flash_addr = (addr / num_effective_busses(s));
         int slave = flash_addr >> LQSPI_ADDRESS_BITS;
@@ -484,14 +494,14 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         for (i = 0; i < LQSPI_CACHE_SIZE / 4; ++i) {
             tx_data_bytes(s, 0, 4);
             xilinx_spips_flush_txfifo(s);
-            rx_data_bytes(s, &s->lqspi_buf[cache_entry], 4);
+            rx_data_bytes(s, &q->lqspi_buf[cache_entry], 4);
             cache_entry++;
         }
 
         s->regs[R_CONFIG] |= CS;
         xilinx_spips_update_cs_lines(s);
 
-        s->lqspi_cached_addr = addr;
+        q->lqspi_cached_addr = addr;
         return lqspi_read(opaque, addr, size);
     }
 }
@@ -511,7 +521,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     int i;
 
-    DB_PRINT("inited device model\n");
+    DB_PRINT("realized spips\n");
 
     s->spi = g_new(SSIBus *, s->num_busses);
     for (i = 0; i < s->num_busses; ++i) {
@@ -531,17 +541,32 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->iomem, &spips_ops, s, "spi", R_MAX*4);
     sysbus_init_mmio(sbd, &s->iomem);
 
-    memory_region_init_io(&s->mmlqspi, &lqspi_ops, s, "lqspi",
-                          (1 << LQSPI_ADDRESS_BITS) * 2);
-    sysbus_init_mmio(sbd, &s->mmlqspi);
-
     s->irqline = -1;
-    s->lqspi_cached_addr = ~0ULL;
 
     fifo8_create(&s->rx_fifo, RXFF_A);
     fifo8_create(&s->tx_fifo, TXFF_A);
 }
 
+static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
+{
+    XilinxSPIPS *s = XILINX_SPIPS(dev);
+    XilinxQSPIPS *q = XILINX_QSPIPS(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    DB_PRINT("realized qspips\n");
+
+    s->num_busses = 2;
+    s->num_cs = 2;
+    s->num_txrx_bytes = 4;
+
+    xilinx_spips_realize(dev, errp);
+    memory_region_init_io(&s->mmlqspi, &lqspi_ops, s, "lqspi",
+                          (1 << LQSPI_ADDRESS_BITS) * 2);
+    sysbus_init_mmio(sbd, &s->mmlqspi);
+
+    q->lqspi_cached_addr = ~0ULL;
+}
+
 static int xilinx_spips_post_load(void *opaque, int version_id)
 {
     xilinx_spips_update_ixr((XilinxSPIPS *)opaque);
@@ -570,6 +595,14 @@ static Property xilinx_spips_properties[] = {
     DEFINE_PROP_UINT8("num-txrx-bytes", XilinxSPIPS, num_txrx_bytes, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
+
+static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = xilinx_qspips_realize;
+}
+
 static void xilinx_spips_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -587,9 +620,17 @@ static const TypeInfo xilinx_spips_info = {
     .class_init = xilinx_spips_class_init,
 };
 
+static const TypeInfo xilinx_qspips_info = {
+    .name  = TYPE_XILINX_QSPIPS,
+    .parent = TYPE_XILINX_SPIPS,
+    .instance_size  = sizeof(XilinxQSPIPS),
+    .class_init = xilinx_qspips_class_init,
+};
+
 static void xilinx_spips_register_types(void)
 {
     type_register_static(&xilinx_spips_info);
+    type_register_static(&xilinx_qspips_info);
 }
 
 type_init(xilinx_spips_register_types)
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 02/15] xilinx_spips: Make interrupts clear on read

[Peter Crosthwaite]

By default these interrupts are clear on read.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 734adf0..261d948 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -330,6 +330,10 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
         mask = 0x0002FFFF;
         break;
     case R_INTR_STATUS:
+        ret = s->regs[addr] & IXR_ALL;
+        s->regs[addr] = 0;
+        DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
+        return ret;
     case R_INTR_MASK:
         mask = IXR_ALL;
         break;
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 03/15] xilinx_spips: Inhibit interrupts in LQSPI mode

[Peter Crosthwaite]

The real hardware does not produce interrupts in LQSPI mode. Inhibit
generation of interrupts when the LQ_MODE bit is set.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 261d948..a8691d5 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -204,6 +204,9 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
 
 static void xilinx_spips_update_ixr(XilinxSPIPS *s)
 {
+    if (s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE) {
+        return;
+    }
     /* These are set/cleared as they occur */
     s->regs[R_INTR_STATUS] &= (IXR_TX_FIFO_UNDERFLOW | IXR_RX_FIFO_OVERFLOW |
                                 IXR_TX_FIFO_MODE_FAIL);
@@ -256,7 +259,9 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
         for (i = 0; i < num_effective_busses(s); ++i) {
             if (!i || s->snoop_state == SNOOP_STRIPING) {
                 if (fifo8_is_empty(&s->tx_fifo)) {
-                    s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
+                    if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) {
+                        s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
+                    }
                     xilinx_spips_update_ixr(s);
                     return;
                 } else {
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 04/15] xilinx_spips: Add verbose LQSPI debug output

[Peter Crosthwaite]

You really need this is you want to track a guest banging on LQSPI.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index a8691d5..29636ce 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -456,10 +456,13 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
     int i;
     XilinxQSPIPS *q = opaque;
     XilinxSPIPS *s = opaque;
+    uint32_t ret;
 
     if (addr >= q->lqspi_cached_addr &&
             addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
-        return q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
+        ret = q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
+        DB_PRINT("addr: %08x, data: %08x\n", (unsigned)addr, (unsigned)ret);
+        return ret;
     } else {
         int flash_addr = (addr / num_effective_busses(s));
         int slave = flash_addr >> LQSPI_ADDRESS_BITS;
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 05/15] xilinx_spips: lqspi: Dont trash config register

[Peter Crosthwaite]

The LQSPI code currently manipulates the config register to achieve its
ends. Some (agressively designed) drivers assume that the config
register preserves state across a transition into and out of LQSPI
mode. Fixed by just restoring R_CONFIG to its original value after
LQSPI does its thing.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 29636ce..06c2ec5 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -467,6 +467,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         int flash_addr = (addr / num_effective_busses(s));
         int slave = flash_addr >> LQSPI_ADDRESS_BITS;
         int cache_entry = 0;
+        uint32_t r_config_save = s->regs[R_CONFIG];
 
         DB_PRINT("config reg status: %08x\n", s->regs[R_LQSPI_CFG]);
 
@@ -512,6 +513,8 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 
         s->regs[R_CONFIG] |= CS;
         xilinx_spips_update_cs_lines(s);
+        s->regs[R_CONFIG] = r_config_save;
+        xilinx_spips_update_cs_lines(s);
 
         q->lqspi_cached_addr = addr;
         return lqspi_read(opaque, addr, size);
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 06/15] xilinx_spips: Fix QSPI FIFO size

[Peter Crosthwaite]

QSPI has a bigger FIFO than the regular SPI controller. Differentiate
between the two with correct FIFO sizes for each.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 06c2ec5..78a3fec 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -106,6 +106,9 @@
 #define RXFF_A          32
 #define TXFF_A          32
 
+#define RXFF_A_Q          (64 * 4)
+#define TXFF_A_Q          (64 * 4)
+
 /* 16MB per linear region */
 #define LQSPI_ADDRESS_BITS 24
 /* Bite off 4k chunks at a time */
@@ -575,6 +578,10 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
     s->num_txrx_bytes = 4;
 
     xilinx_spips_realize(dev, errp);
+    fifo8_destroy(&s->rx_fifo);
+    fifo8_destroy(&s->tx_fifo);
+    fifo8_create(&s->rx_fifo, RXFF_A_Q);
+    fifo8_create(&s->tx_fifo, TXFF_A_Q);
     memory_region_init_io(&s->mmlqspi, &lqspi_ops, s, "lqspi",
                           (1 << LQSPI_ADDRESS_BITS) * 2);
     sysbus_init_mmio(sbd, &s->mmlqspi);
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 07/15] xilinx_spips: Trash LQ page cache on mode change

[Peter Crosthwaite]

Invalidate the LQSPI cached page when transitioning into LQSPI mode.
Otherwise there is a possibility that the controller will return stale
data to the guest when transitioning back to LQ_MODE after a page
program.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 78a3fec..4f921f8 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -390,6 +390,9 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
     int mask = ~0;
     int man_start_com = 0;
     XilinxSPIPS *s = opaque;
+    /* FIXME: abstract away somehow */
+    XilinxQSPIPS *q = (XilinxQSPIPS *)object_dynamic_cast(OBJECT(s),
+                       TYPE_XILINX_QSPIPS);
 
     DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr, (unsigned)value);
     addr >>= 2;
@@ -435,6 +438,11 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
     case R_TXD3:
         tx_data_bytes(s, (uint32_t)value, 3);
         goto no_reg_update;
+    case R_LQSPI_CFG:
+        if (q) {
+            q->lqspi_cached_addr = ~0ULL;
+        }
+        break;
     }
     s->regs[addr] = (s->regs[addr] & ~mask) | (value & mask);
 no_reg_update:
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 08/15] xilinx_spips: Add automatic start support

[Peter Crosthwaite]

SPI has a mode where it automatically starts based on tx fifo
occupancy. Implemented.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 4f921f8..f7d942e 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -446,7 +446,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
     }
     s->regs[addr] = (s->regs[addr] & ~mask) | (value & mask);
 no_reg_update:
-    if (man_start_com) {
+    if ((man_start_com && s->regs[R_CONFIG] & MAN_START_EN) ||
+            (fifo8_is_empty(&s->tx_fifo) && s->regs[R_CONFIG] & MAN_START_EN)) {
         xilinx_spips_flush_txfifo(s);
     }
     xilinx_spips_update_ixr(s);
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 09/15] xilinx_spips: Implement automatic CS

[Peter Crosthwaite]

Implement the automatic CS control feature. If the MANUAL_CS bit is
cleared then the chip select stay de-asserted as long as the tx FIFO
is empty.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index f7d942e..16c2e1d 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -177,6 +177,12 @@ static inline int num_effective_busses(XilinxSPIPS *s)
             s->regs[R_LQSPI_CFG] & LQSPI_CFG_TWO_MEM) ? s->num_busses : 1;
 }
 
+static inline bool xilinx_spips_cs_is_set(XilinxSPIPS *s, int i, int field)
+{
+    return ~field & (1 << i) && (s->regs[R_CONFIG] & MANUAL_CS
+                    || !fifo8_is_empty(&s->tx_fifo));
+}
+
 static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
 {
     int i, j;
@@ -189,14 +195,14 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
             int cs_to_set = (j * s->num_cs + i + upage) %
                                 (s->num_cs * s->num_busses);
 
-            if (~field & (1 << i) && !found) {
+            if (xilinx_spips_cs_is_set(s, i, field) && !found) {
                 DB_PRINT("selecting slave %d\n", i);
                 qemu_set_irq(s->cs_lines[cs_to_set], 0);
             } else {
                 qemu_set_irq(s->cs_lines[cs_to_set], 1);
             }
         }
-        if (~field & (1 << i)) {
+        if (xilinx_spips_cs_is_set(s, i, field)) {
             found = true;
         }
     }
@@ -487,7 +493,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         fifo8_reset(&s->rx_fifo);
 
         s->regs[R_CONFIG] &= ~CS;
-        s->regs[R_CONFIG] |= (~(1 << slave) << CS_SHIFT) & CS;
+        s->regs[R_CONFIG] |= ((~(1 << slave) << CS_SHIFT) & CS) | MANUAL_CS;
         xilinx_spips_update_cs_lines(s);
 
         /* instruction */
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 10/15] xilinx_spips: Fix CTRL register RW bits

[Peter Crosthwaite]

The CTRL register was RAZ/WI on some of the RW bits. Even though the
function behind these bits is invalid in QEMU, they should still be
guest accessible. Fix.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 16c2e1d..a2019e4 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -341,7 +341,7 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
     addr >>= 2;
     switch (addr) {
     case R_CONFIG:
-        mask = 0x0002FFFF;
+        mask = 0x840AFFFF;
         break;
     case R_INTR_STATUS:
         ret = s->regs[addr] & IXR_ALL;
@@ -404,7 +404,7 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
     addr >>= 2;
     switch (addr) {
     case R_CONFIG:
-        mask = 0x0002FFFF;
+        mask = 0x840AFFFF;
         if (value & MAN_START_COM) {
             man_start_com = 1;
         }
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 11/15] xilinx_spips: Fix striping behaviour

[Peter Crosthwaite]

The QSPI controller was using byte-wide stripes when striping across
the two flashes in dual parallel mode. The real hardware however uses
individual bit striping. QEMU misbehaves in the (corner) case where
data is written/read in dual-parallel mode and read/written back in
single mode.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |   74 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index a2019e4..43ce2d8 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -258,35 +258,67 @@ static void xilinx_spips_reset(DeviceState *d)
     xilinx_spips_update_cs_lines(s);
 }
 
+static inline void stripe8(uint8_t *x, int num, bool dir)
+{
+    uint8_t r[num];
+    memset(r, 0, sizeof(uint8_t) * num);
+    int idx[2] = {0, 0};
+    int bit[2] = {0, 0};
+    int d = dir;
+
+    for (idx[0] = 0; idx[0] < num; ++idx[0]) {
+        for (bit[0] = 0; bit[0] < 8; ++bit[0]) {
+            r[idx[d]] |= x[idx[!d]] & 1 << bit[!d] ? 1 << bit[d] : 0;
+            idx[1] = (idx[1] + 1) % num;
+            if (!idx[1]) {
+                bit[1]++;
+            }
+        }
+    }
+    memcpy(x, r, sizeof(uint8_t) * num);
+}
+
 static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
 {
     for (;;) {
         int i;
-        uint8_t rx;
         uint8_t tx = 0;
+        uint8_t tx_rx[num_effective_busses(s)];
 
-        for (i = 0; i < num_effective_busses(s); ++i) {
-            if (!i || s->snoop_state == SNOOP_STRIPING) {
-                if (fifo8_is_empty(&s->tx_fifo)) {
-                    if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) {
-                        s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
-                    }
-                    xilinx_spips_update_ixr(s);
-                    return;
-                } else {
-                    tx = fifo8_pop(&s->tx_fifo);
-                }
+        if (fifo8_is_empty(&s->tx_fifo)) {
+            if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) {
+                s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
+            }
+            xilinx_spips_update_ixr(s);
+            return;
+        } else if (s->snoop_state == SNOOP_STRIPING) {
+            for (i = 0; i < num_effective_busses(s); ++i) {
+                tx_rx[i] = fifo8_pop(&s->tx_fifo);
             }
-            rx = ssi_transfer(s->spi[i], (uint32_t)tx);
-            DB_PRINT("tx = %02x rx = %02x\n", tx, rx);
-            if (!i || s->snoop_state == SNOOP_STRIPING) {
-                if (fifo8_is_full(&s->rx_fifo)) {
-                    s->regs[R_INTR_STATUS] |= IXR_RX_FIFO_OVERFLOW;
-                    DB_PRINT("rx FIFO overflow");
-                } else {
-                    fifo8_push(&s->rx_fifo, (uint8_t)rx);
-                }
+            stripe8(tx_rx, num_effective_busses(s), false);
+        } else {
+            tx = fifo8_pop(&s->tx_fifo);
+            for (i = 0; i < num_effective_busses(s); ++i) {
+                tx_rx[i] = tx;
+            }
+        }
+
+        for (i = 0; i < num_effective_busses(s); ++i) {
+            DB_PRINT("tx = %02x\n", tx_rx[i]);
+            tx_rx[i] = ssi_transfer(s->spi[i], (uint32_t)tx_rx[i]);
+            DB_PRINT("rx = %02x\n", tx_rx[i]);
+        }
+
+        if (fifo8_is_full(&s->rx_fifo)) {
+            s->regs[R_INTR_STATUS] |= IXR_RX_FIFO_OVERFLOW;
+            DB_PRINT("rx FIFO overflow");
+        } else if (s->snoop_state == SNOOP_STRIPING) {
+            stripe8(tx_rx, num_effective_busses(s), true);
+            for (i = 0; i < num_effective_busses(s); ++i) {
+                fifo8_push(&s->rx_fifo, (uint8_t)tx_rx[i]);
             }
+        } else {
+           fifo8_push(&s->rx_fifo, (uint8_t)tx_rx[0]);
         }
 
         switch (s->snoop_state) {
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 12/15] xilinx_spips: Debug msgs for Snoop state

[Peter Crosthwaite]

This is worth keeping track of when debugging the device model.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 43ce2d8..0d01103 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -208,6 +208,7 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
     }
     if (!found) {
         s->snoop_state = SNOOP_CHECKING;
+        DB_PRINT("moving to snoop check state\n");
     }
 }
 
@@ -321,6 +322,7 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
            fifo8_push(&s->rx_fifo, (uint8_t)tx_rx[0]);
         }
 
+        DB_PRINT("initial snoop state: %x\n", (unsigned)s->snoop_state);
         switch (s->snoop_state) {
         case (SNOOP_CHECKING):
             switch (tx) { /* new instruction code */
@@ -349,6 +351,7 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
         default:
             s->snoop_state--;
         }
+        DB_PRINT("final snoop state: %x\n", (unsigned)s->snoop_state);
     }
 }
 
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 13/15] xilinx_spips: Multiple debug verbosity levels

[Peter Crosthwaite]

The debug printfs on every SPI operation is extremely verbose. Add
a second level of debug for this.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |   68 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 41 insertions(+), 27 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 0d01103..5ac0f64 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -30,15 +30,17 @@
 #include "hw/ssi.h"
 #include "qemu/bitops.h"
 
-#ifdef XILINX_SPIPS_ERR_DEBUG
-#define DB_PRINT(...) do { \
-    fprintf(stderr,  ": %s: ", __func__); \
-    fprintf(stderr, ## __VA_ARGS__); \
-    } while (0);
-#else
-    #define DB_PRINT(...)
+#ifndef XILINX_SPIPS_ERR_DEBUG
+#define XILINX_SPIPS_ERR_DEBUG 0
 #endif
 
+#define DB_PRINT_L(level, ...) do { \
+    if (XILINX_SPIPS_ERR_DEBUG > (level)) { \
+        fprintf(stderr,  ": %s: ", __func__); \
+        fprintf(stderr, ## __VA_ARGS__); \
+    } \
+} while (0);
+
 /* config register */
 #define R_CONFIG            (0x00 / 4)
 #define IFMODE              (1 << 31)
@@ -196,7 +198,7 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
                                 (s->num_cs * s->num_busses);
 
             if (xilinx_spips_cs_is_set(s, i, field) && !found) {
-                DB_PRINT("selecting slave %d\n", i);
+                DB_PRINT_L(0, "selecting slave %d\n", i);
                 qemu_set_irq(s->cs_lines[cs_to_set], 0);
             } else {
                 qemu_set_irq(s->cs_lines[cs_to_set], 1);
@@ -208,7 +210,7 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
     }
     if (!found) {
         s->snoop_state = SNOOP_CHECKING;
-        DB_PRINT("moving to snoop check state\n");
+        DB_PRINT_L(1, "moving to snoop check state\n");
     }
 }
 
@@ -281,6 +283,8 @@ static inline void stripe8(uint8_t *x, int num, bool dir)
 
 static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
 {
+    int debug_level = 0;
+
     for (;;) {
         int i;
         uint8_t tx = 0;
@@ -305,14 +309,14 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
         }
 
         for (i = 0; i < num_effective_busses(s); ++i) {
-            DB_PRINT("tx = %02x\n", tx_rx[i]);
+            DB_PRINT_L(debug_level, "tx = %02x\n", tx_rx[i]);
             tx_rx[i] = ssi_transfer(s->spi[i], (uint32_t)tx_rx[i]);
-            DB_PRINT("rx = %02x\n", tx_rx[i]);
+            DB_PRINT_L(debug_level, "rx = %02x\n", tx_rx[i]);
         }
 
         if (fifo8_is_full(&s->rx_fifo)) {
             s->regs[R_INTR_STATUS] |= IXR_RX_FIFO_OVERFLOW;
-            DB_PRINT("rx FIFO overflow");
+            DB_PRINT_L(0, "rx FIFO overflow");
         } else if (s->snoop_state == SNOOP_STRIPING) {
             stripe8(tx_rx, num_effective_busses(s), true);
             for (i = 0; i < num_effective_busses(s); ++i) {
@@ -322,7 +326,8 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
            fifo8_push(&s->rx_fifo, (uint8_t)tx_rx[0]);
         }
 
-        DB_PRINT("initial snoop state: %x\n", (unsigned)s->snoop_state);
+        DB_PRINT_L(debug_level, "initial snoop state: %x\n",
+                   (unsigned)s->snoop_state);
         switch (s->snoop_state) {
         case (SNOOP_CHECKING):
             switch (tx) { /* new instruction code */
@@ -347,11 +352,17 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
             break;
         case (SNOOP_STRIPING):
         case (SNOOP_NONE):
+            /* Once we hit the boring stuff - squelch debug noise */
+            if (!debug_level) {
+                DB_PRINT_L(0, "squelching debug info ....\n");
+                debug_level = 1;
+            }
             break;
         default:
             s->snoop_state--;
         }
-        DB_PRINT("final snoop state: %x\n", (unsigned)s->snoop_state);
+        DB_PRINT_L(debug_level, "final snoop state: %x\n",
+                   (unsigned)s->snoop_state);
     }
 }
 
@@ -381,7 +392,7 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
     case R_INTR_STATUS:
         ret = s->regs[addr] & IXR_ALL;
         s->regs[addr] = 0;
-        DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
+        DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
         return ret;
     case R_INTR_MASK:
         mask = IXR_ALL;
@@ -402,11 +413,12 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
         break;
     case R_RX_DATA:
         rx_data_bytes(s, &ret, s->num_txrx_bytes);
-        DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
+        DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
         xilinx_spips_update_ixr(s);
         return ret;
     }
-    DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr * 4, s->regs[addr] & mask);
+    DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr * 4,
+               s->regs[addr] & mask);
     return s->regs[addr] & mask;
 
 }
@@ -435,7 +447,7 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
     XilinxQSPIPS *q = (XilinxQSPIPS *)object_dynamic_cast(OBJECT(s),
                        TYPE_XILINX_QSPIPS);
 
-    DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr, (unsigned)value);
+    DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr, (unsigned)value);
     addr >>= 2;
     switch (addr) {
     case R_CONFIG:
@@ -514,7 +526,8 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
     if (addr >= q->lqspi_cached_addr &&
             addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
         ret = q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
-        DB_PRINT("addr: %08x, data: %08x\n", (unsigned)addr, (unsigned)ret);
+        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
+                   (unsigned)ret);
         return ret;
     } else {
         int flash_addr = (addr / num_effective_busses(s));
@@ -522,7 +535,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         int cache_entry = 0;
         uint32_t r_config_save = s->regs[R_CONFIG];
 
-        DB_PRINT("config reg status: %08x\n", s->regs[R_LQSPI_CFG]);
+        DB_PRINT_L(0, "config reg status: %08x\n", s->regs[R_LQSPI_CFG]);
 
         fifo8_reset(&s->tx_fifo);
         fifo8_reset(&s->rx_fifo);
@@ -532,11 +545,12 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         xilinx_spips_update_cs_lines(s);
 
         /* instruction */
-        DB_PRINT("pushing read instruction: %02x\n",
-                 (uint8_t)(s->regs[R_LQSPI_CFG] & LQSPI_CFG_INST_CODE));
+        DB_PRINT_L(0, "pushing read instruction: %02x\n",
+                   (unsigned)(uint8_t)(s->regs[R_LQSPI_CFG] &
+                                       LQSPI_CFG_INST_CODE));
         fifo8_push(&s->tx_fifo, s->regs[R_LQSPI_CFG] & LQSPI_CFG_INST_CODE);
         /* read address */
-        DB_PRINT("pushing read address %06x\n", flash_addr);
+        DB_PRINT_L(0, "pushing read address %06x\n", flash_addr);
         fifo8_push(&s->tx_fifo, (uint8_t)(flash_addr >> 16));
         fifo8_push(&s->tx_fifo, (uint8_t)(flash_addr >> 8));
         fifo8_push(&s->tx_fifo, (uint8_t)flash_addr);
@@ -549,13 +563,13 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         /* dummy bytes */
         for (i = 0; i < (extract32(s->regs[R_LQSPI_CFG], LQSPI_CFG_DUMMY_SHIFT,
                                    LQSPI_CFG_DUMMY_WIDTH)); ++i) {
-            DB_PRINT("pushing dummy byte\n");
+            DB_PRINT_L(0, "pushing dummy byte\n");
             fifo8_push(&s->tx_fifo, 0);
         }
         xilinx_spips_flush_txfifo(s);
         fifo8_reset(&s->rx_fifo);
 
-        DB_PRINT("starting QSPI data read\n");
+        DB_PRINT_L(0, "starting QSPI data read\n");
 
         for (i = 0; i < LQSPI_CACHE_SIZE / 4; ++i) {
             tx_data_bytes(s, 0, 4);
@@ -589,7 +603,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     int i;
 
-    DB_PRINT("realized spips\n");
+    DB_PRINT_L(0, "realized spips\n");
 
     s->spi = g_new(SSIBus *, s->num_busses);
     for (i = 0; i < s->num_busses; ++i) {
@@ -621,7 +635,7 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
     XilinxQSPIPS *q = XILINX_QSPIPS(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    DB_PRINT("realized qspips\n");
+    DB_PRINT_L(0, "realized qspips\n");
 
     s->num_busses = 2;
     s->num_cs = 2;
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 14/15] xilinx_spips: lqspi: Push more data to tx-fifo

[Peter Crosthwaite]

Do 16 words per fifo flush. Increases performance and decreases
debug verbosity. This data depth has no real hardware analogue,
so just go with something that has reasonable performance.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 5ac0f64..32d8db8 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -571,11 +571,14 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 
         DB_PRINT_L(0, "starting QSPI data read\n");
 
-        for (i = 0; i < LQSPI_CACHE_SIZE / 4; ++i) {
-            tx_data_bytes(s, 0, 4);
+        while (cache_entry < LQSPI_CACHE_SIZE / 4) {
+            for (i = 0; i < 16; ++i) {
+                tx_data_bytes(s, 0, 4);
+            }
             xilinx_spips_flush_txfifo(s);
-            rx_data_bytes(s, &q->lqspi_buf[cache_entry], 4);
-            cache_entry++;
+            for (i = 0; i < 16; ++i) {
+                rx_data_bytes(s, &q->lqspi_buf[cache_entry++], 4);
+            }
         }
 
         s->regs[R_CONFIG] |= CS;
-- 
1.7.0.4

[Qemu-devel] [PATCH arm-devs v1 15/15] xilinx_spips: lqspi: Fix byte/misaligned access

[Peter Crosthwaite]

The LQSPI bus attachment supports byte/halfword and misaligned
accesses. Fixed. Refactored the LQSPI cache to be byte-wise
instead of word wise accordingly.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/xilinx_spips.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
index 32d8db8..cb45a9c 100644
--- a/hw/xilinx_spips.c
+++ b/hw/xilinx_spips.c
@@ -160,7 +160,7 @@ typedef struct {
 typedef struct {
     XilinxSPIPS parent_obj;
 
-    uint32_t lqspi_buf[LQSPI_CACHE_SIZE];
+    uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
     hwaddr lqspi_cached_addr;
 } XilinxQSPIPS;
 
@@ -366,14 +366,12 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
     }
 }
 
-static inline void rx_data_bytes(XilinxSPIPS *s, uint32_t *value, int max)
+static inline void rx_data_bytes(XilinxSPIPS *s, uint8_t *value, int max)
 {
     int i;
 
-    *value = 0;
     for (i = 0; i < max && !fifo8_is_empty(&s->rx_fifo); ++i) {
-        uint32_t next = fifo8_pop(&s->rx_fifo) & 0xFF;
-        *value |= next << 8 * (s->regs[R_CONFIG] & ENDIAN ? 3-i : i);
+        value[i] = fifo8_pop(&s->rx_fifo);
     }
 }
 
@@ -383,6 +381,7 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
     XilinxSPIPS *s = opaque;
     uint32_t mask = ~0;
     uint32_t ret;
+    uint8_t rx_buf[4];
 
     addr >>= 2;
     switch (addr) {
@@ -412,7 +411,10 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
         mask = 0;
         break;
     case R_RX_DATA:
-        rx_data_bytes(s, &ret, s->num_txrx_bytes);
+        memset(rx_buf, 0, sizeof(rx_buf));
+        rx_data_bytes(s, rx_buf, s->num_txrx_bytes);
+        ret = s->regs[R_CONFIG] & ENDIAN ? cpu_to_be32(*(uint32_t *)rx_buf)
+                        : cpu_to_le32(*(uint32_t *)rx_buf);
         DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
         xilinx_spips_update_ixr(s);
         return ret;
@@ -525,7 +527,8 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 
     if (addr >= q->lqspi_cached_addr &&
             addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
-        ret = q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
+        uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
+        ret = cpu_to_le32(*(uint32_t *)retp);
         DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
                    (unsigned)ret);
         return ret;
@@ -571,13 +574,13 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 
         DB_PRINT_L(0, "starting QSPI data read\n");
 
-        while (cache_entry < LQSPI_CACHE_SIZE / 4) {
-            for (i = 0; i < 16; ++i) {
-                tx_data_bytes(s, 0, 4);
+        while (cache_entry < LQSPI_CACHE_SIZE) {
+            for (i = 0; i < 64; ++i) {
+                tx_data_bytes(s, 0, 1);
             }
             xilinx_spips_flush_txfifo(s);
-            for (i = 0; i < 16; ++i) {
-                rx_data_bytes(s, &q->lqspi_buf[cache_entry++], 4);
+            for (i = 0; i < 64; ++i) {
+                rx_data_bytes(s, &q->lqspi_buf[cache_entry++], 1);
             }
         }
 
@@ -586,7 +589,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         s->regs[R_CONFIG] = r_config_save;
         xilinx_spips_update_cs_lines(s);
 
-        q->lqspi_cached_addr = addr;
+        q->lqspi_cached_addr = flash_addr * num_effective_busses(s);
         return lqspi_read(opaque, addr, size);
     }
 }
@@ -595,7 +598,7 @@ static const MemoryRegionOps lqspi_ops = {
     .read = lqspi_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
-        .min_access_size = 4,
+        .min_access_size = 1,
         .max_access_size = 4
     }
 };
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH arm-devs v1 03/15] xilinx_spips: Inhibit interrupts in LQSPI mode

[Peter Maydell]

On 3 April 2013 05:27, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> The real hardware does not produce interrupts in LQSPI mode. Inhibit
> generation of interrupts when the LQ_MODE bit is set.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index 261d948..a8691d5 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -204,6 +204,9 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
>
>  static void xilinx_spips_update_ixr(XilinxSPIPS *s)
>  {
> +    if (s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE) {
> +        return;
> +    }
>      /* These are set/cleared as they occur */
>      s->regs[R_INTR_STATUS] &= (IXR_TX_FIFO_UNDERFLOW | IXR_RX_FIFO_OVERFLOW |
>                                  IXR_TX_FIFO_MODE_FAIL);
> @@ -256,7 +259,9 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
>          for (i = 0; i < num_effective_busses(s); ++i) {
>              if (!i || s->snoop_state == SNOOP_STRIPING) {
>                  if (fifo8_is_empty(&s->tx_fifo)) {
> -                    s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
> +                    if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) {
> +                        s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
> +                    }
>                      xilinx_spips_update_ixr(s);
>                      return;
>                  } else {

How about the OVERFLOW case just below here, or is that a
deliberate omission?

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 04/15] xilinx_spips: Add verbose LQSPI debug output

[Peter Maydell]

On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> You really need this is you want to track a guest banging on LQSPI.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index a8691d5..29636ce 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -456,10 +456,13 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>      int i;
>      XilinxQSPIPS *q = opaque;
>      XilinxSPIPS *s = opaque;
> +    uint32_t ret;
>
>      if (addr >= q->lqspi_cached_addr &&
>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
> -        return q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
> +        ret = q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
> +        DB_PRINT("addr: %08x, data: %08x\n", (unsigned)addr, (unsigned)ret);
> +        return ret;
>      } else {
>          int flash_addr = (addr / num_effective_busses(s));
>          int slave = flash_addr >> LQSPI_ADDRESS_BITS;

I'd recommend keeping the scope of the variable inside the if {} personally,
but if you prefer this way round that's fine.

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 05/15] xilinx_spips: lqspi: Dont trash config register

[Peter Maydell]

On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> The LQSPI code currently manipulates the config register to achieve its
> ends. Some (agressively designed) drivers assume that the config
> register preserves state across a transition into and out of LQSPI
> mode. Fixed by just restoring R_CONFIG to its original value after
> LQSPI does its thing.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index 29636ce..06c2ec5 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -467,6 +467,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>          int flash_addr = (addr / num_effective_busses(s));
>          int slave = flash_addr >> LQSPI_ADDRESS_BITS;
>          int cache_entry = 0;
> +        uint32_t r_config_save = s->regs[R_CONFIG];
>
>          DB_PRINT("config reg status: %08x\n", s->regs[R_LQSPI_CFG]);
>
> @@ -512,6 +513,8 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>
>          s->regs[R_CONFIG] |= CS;
>          xilinx_spips_update_cs_lines(s);
> +        s->regs[R_CONFIG] = r_config_save;
> +        xilinx_spips_update_cs_lines(s);
>
>          q->lqspi_cached_addr = addr;
>          return lqspi_read(opaque, addr, size);
> --
> 1.7.0.4

Is this a "we don't implement this the way the hardware does but
this is close enough" kind of thing? (in particular does the hardware
really do the same thing to the cs lines that those two calls to
update_cs_lines() presumably do?) Maybe worth a comment in the code
about what we do vs what hardware does / what we theoretically
ideally should do, if you happen to know.

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 06/15] xilinx_spips: Fix QSPI FIFO size

[Peter Maydell]

On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> QSPI has a bigger FIFO than the regular SPI controller. Differentiate
> between the two with correct FIFO sizes for each.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index 06c2ec5..78a3fec 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -106,6 +106,9 @@
>  #define RXFF_A          32
>  #define TXFF_A          32
>
> +#define RXFF_A_Q          (64 * 4)
> +#define TXFF_A_Q          (64 * 4)
> +
>  /* 16MB per linear region */
>  #define LQSPI_ADDRESS_BITS 24
>  /* Bite off 4k chunks at a time */
> @@ -575,6 +578,10 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
>      s->num_txrx_bytes = 4;
>
>      xilinx_spips_realize(dev, errp);
> +    fifo8_destroy(&s->rx_fifo);
> +    fifo8_destroy(&s->tx_fifo);
> +    fifo8_create(&s->rx_fifo, RXFF_A_Q);
> +    fifo8_create(&s->tx_fifo, TXFF_A_Q);
>      memory_region_init_io(&s->mmlqspi, &lqspi_ops, s, "lqspi",
>                            (1 << LQSPI_ADDRESS_BITS) * 2);
>      sysbus_init_mmio(sbd, &s->mmlqspi);

Destroying and recreating the fifos seems a bit odd -- can you
structure this so the base class instance init just creates them
at the right size? I guess the obvious way would be to have the
class struct have a field for fifo size which the class init
function sets appropriately, and then instance init creates
a fifo of that size. This is kind of like how target-arm/cpu.c
handles ID register fields, but in the class rather than the
instance struct.

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 07/15] xilinx_spips: Trash LQ page cache on mode change

[Peter Maydell]

On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Invalidate the LQSPI cached page when transitioning into LQSPI mode.
> Otherwise there is a possibility that the controller will return stale
> data to the guest when transitioning back to LQ_MODE after a page
> program.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index 78a3fec..4f921f8 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -390,6 +390,9 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>      int mask = ~0;
>      int man_start_com = 0;
>      XilinxSPIPS *s = opaque;
> +    /* FIXME: abstract away somehow */
> +    XilinxQSPIPS *q = (XilinxQSPIPS *)object_dynamic_cast(OBJECT(s),
> +                       TYPE_XILINX_QSPIPS);
>

Hmm, a C cast of the result of an object_dynamic_cast()?
Would be nice to do this some other way... Maybe a member
function (ie fn ptr in the class struct) which does nothing
on SPIPS and clears the cached page on QSPIPS?

>      DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr, (unsigned)value);
>      addr >>= 2;
> @@ -435,6 +438,11 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>      case R_TXD3:
>          tx_data_bytes(s, (uint32_t)value, 3);
>          goto no_reg_update;
> +    case R_LQSPI_CFG:
> +        if (q) {
> +            q->lqspi_cached_addr = ~0ULL;
> +        }
> +        break;
>      }
>      s->regs[addr] = (s->regs[addr] & ~mask) | (value & mask);
>  no_reg_update:
> --
> 1.7.0.4
>

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 10/15] xilinx_spips: Fix CTRL register RW bits

[Peter Maydell]

On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> The CTRL register was RAZ/WI on some of the RW bits. Even though the
> function behind these bits is invalid in QEMU, they should still be
> guest accessible. Fix.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index 16c2e1d..a2019e4 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -341,7 +341,7 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
>      addr >>= 2;
>      switch (addr) {
>      case R_CONFIG:
> -        mask = 0x0002FFFF;
> +        mask = 0x840AFFFF;
>          break;
>      case R_INTR_STATUS:
>          ret = s->regs[addr] & IXR_ALL;
> @@ -404,7 +404,7 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>      addr >>= 2;
>      switch (addr) {
>      case R_CONFIG:
> -        mask = 0x0002FFFF;
> +        mask = 0x840AFFFF;
>          if (value & MAN_START_COM) {
>              man_start_com = 1;
>          }
> --
> 1.7.0.4
>


Maybe the magic number for 'implemented bits in the register'
deserves a #define ?

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 11/15] xilinx_spips: Fix striping behaviour

[Peter Maydell]

On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> The QSPI controller was using byte-wide stripes when striping across
> the two flashes in dual parallel mode. The real hardware however uses
> individual bit striping. QEMU misbehaves in the (corner) case where
> data is written/read in dual-parallel mode and read/written back in
> single mode.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |   74 ++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 53 insertions(+), 21 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index a2019e4..43ce2d8 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -258,35 +258,67 @@ static void xilinx_spips_reset(DeviceState *d)
>      xilinx_spips_update_cs_lines(s);
>  }
>
> +static inline void stripe8(uint8_t *x, int num, bool dir)
> +{
> +    uint8_t r[num];
> +    memset(r, 0, sizeof(uint8_t) * num);

Don't interleave code and variable definitions, please.

> +    int idx[2] = {0, 0};
> +    int bit[2] = {0, 0};
> +    int d = dir;
> +
> +    for (idx[0] = 0; idx[0] < num; ++idx[0]) {
> +        for (bit[0] = 0; bit[0] < 8; ++bit[0]) {
> +            r[idx[d]] |= x[idx[!d]] & 1 << bit[!d] ? 1 << bit[d] : 0;
> +            idx[1] = (idx[1] + 1) % num;
> +            if (!idx[1]) {
> +                bit[1]++;
> +            }
> +        }
> +    }
> +    memcpy(x, r, sizeof(uint8_t) * num);
> +}

This function could really use a comment saying what it's doing...


thanks
-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 15/15] xilinx_spips: lqspi: Fix byte/misaligned access

[Peter Maydell]

On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> The LQSPI bus attachment supports byte/halfword and misaligned
> accesses. Fixed. Refactored the LQSPI cache to be byte-wise
> instead of word wise accordingly.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/xilinx_spips.c |   31 +++++++++++++++++--------------
>  1 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
> index 32d8db8..cb45a9c 100644
> --- a/hw/xilinx_spips.c
> +++ b/hw/xilinx_spips.c
> @@ -160,7 +160,7 @@ typedef struct {
>  typedef struct {
>      XilinxSPIPS parent_obj;
>
> -    uint32_t lqspi_buf[LQSPI_CACHE_SIZE];
> +    uint8_t lqspi_buf[LQSPI_CACHE_SIZE];

Is it really right that this buffer isn't in the vmstate,
by the way?

>      hwaddr lqspi_cached_addr;
>  } XilinxQSPIPS;

-- PMM

Re: [Qemu-devel] [PATCH arm-devs v1 03/15] xilinx_spips: Inhibit interrupts in LQSPI mode

[Peter Crosthwaite]

Hi Peter,

On Sat, Apr 6, 2013 at 4:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:27, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> The real hardware does not produce interrupts in LQSPI mode. Inhibit
>> generation of interrupts when the LQ_MODE bit is set.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index 261d948..a8691d5 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -204,6 +204,9 @@ static void xilinx_spips_update_cs_lines(XilinxSPIPS *s)
>>
>>  static void xilinx_spips_update_ixr(XilinxSPIPS *s)
>>  {
>> +    if (s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE) {
>> +        return;
>> +    }
>>      /* These are set/cleared as they occur */
>>      s->regs[R_INTR_STATUS] &= (IXR_TX_FIFO_UNDERFLOW | IXR_RX_FIFO_OVERFLOW |
>>                                  IXR_TX_FIFO_MODE_FAIL);
>> @@ -256,7 +259,9 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
>>          for (i = 0; i < num_effective_busses(s); ++i) {
>>              if (!i || s->snoop_state == SNOOP_STRIPING) {
>>                  if (fifo8_is_empty(&s->tx_fifo)) {
>> -                    s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
>> +                    if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) {
>> +                        s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW;
>> +                    }
>>                      xilinx_spips_update_ixr(s);
>>                      return;
>>                  } else {
>
> How about the OVERFLOW case just below here, or is that a
> deliberate omission?
>

Deliberate omission. Overflow in LQPSI mode should be impossible as
LQSPI always resets the fifos and does one pop for every push. You
could make this happen if you tried to do non-linear accesses while in
linear mode but this is undefined behaviour (perhaps worthy of a
LOG_GUEST_ERROR on the register access but that's a separate followup
patch).

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 04/15] xilinx_spips: Add verbose LQSPI debug output

[Peter Crosthwaite]

On Sat, Apr 6, 2013 at 4:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> You really need this is you want to track a guest banging on LQSPI.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index a8691d5..29636ce 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -456,10 +456,13 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>>      int i;
>>      XilinxQSPIPS *q = opaque;
>>      XilinxSPIPS *s = opaque;
>> +    uint32_t ret;
>>
>>      if (addr >= q->lqspi_cached_addr &&
>>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
>> -        return q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
>> +        ret = q->lqspi_buf[(addr - q->lqspi_cached_addr) >> 2];
>> +        DB_PRINT("addr: %08x, data: %08x\n", (unsigned)addr, (unsigned)ret);
>> +        return ret;
>>      } else {
>>          int flash_addr = (addr / num_effective_busses(s));
>>          int slave = flash_addr >> LQSPI_ADDRESS_BITS;
>
> I'd recommend keeping the scope of the variable inside the if {} personally,
> but if you prefer this way round that's fine.
>

I'm usually the same, but for return temporaries I usually define on
top level as they have a habit of getting reused and its verbose if
multiple ifs all define their own ret tempory. All personal pref I
guess.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 05/15] xilinx_spips: lqspi: Dont trash config register

[Peter Crosthwaite]

Hi Peter,

On Sat, Apr 6, 2013 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> The LQSPI code currently manipulates the config register to achieve its
>> ends. Some (agressively designed) drivers assume that the config
>> register preserves state across a transition into and out of LQSPI
>> mode. Fixed by just restoring R_CONFIG to its original value after
>> LQSPI does its thing.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index 29636ce..06c2ec5 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -467,6 +467,7 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>>          int flash_addr = (addr / num_effective_busses(s));
>>          int slave = flash_addr >> LQSPI_ADDRESS_BITS;
>>          int cache_entry = 0;
>> +        uint32_t r_config_save = s->regs[R_CONFIG];
>>
>>          DB_PRINT("config reg status: %08x\n", s->regs[R_LQSPI_CFG]);
>>
>> @@ -512,6 +513,8 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>>
>>          s->regs[R_CONFIG] |= CS;
>>          xilinx_spips_update_cs_lines(s);
>> +        s->regs[R_CONFIG] = r_config_save;
>> +        xilinx_spips_update_cs_lines(s);
>>
>>          q->lqspi_cached_addr = addr;
>>          return lqspi_read(opaque, addr, size);
>> --
>> 1.7.0.4
>
> Is this a "we don't implement this the way the hardware does but
> this is close enough" kind of thing? (in particular does the hardware
> really do the same thing to the cs lines that those two calls to
> update_cs_lines() presumably do?)

No. Turns out I obsoleted by own patch. The Automatic CS feature
(introduced later in the series) is how LQSPI is supposed to work its
CSs. Creating a new patch after the auto CS that fixes this. This
patch is a goner.

Regards,
Peter

> about what we do vs what hardware does / what we theoretically
> ideally should do, if you happen to know.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 06/15] xilinx_spips: Fix QSPI FIFO size

[Peter Crosthwaite]

On Sat, Apr 6, 2013 at 4:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> QSPI has a bigger FIFO than the regular SPI controller. Differentiate
>> between the two with correct FIFO sizes for each.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index 06c2ec5..78a3fec 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -106,6 +106,9 @@
>>  #define RXFF_A          32
>>  #define TXFF_A          32
>>
>> +#define RXFF_A_Q          (64 * 4)
>> +#define TXFF_A_Q          (64 * 4)
>> +
>>  /* 16MB per linear region */
>>  #define LQSPI_ADDRESS_BITS 24
>>  /* Bite off 4k chunks at a time */
>> @@ -575,6 +578,10 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
>>      s->num_txrx_bytes = 4;
>>
>>      xilinx_spips_realize(dev, errp);
>> +    fifo8_destroy(&s->rx_fifo);
>> +    fifo8_destroy(&s->tx_fifo);
>> +    fifo8_create(&s->rx_fifo, RXFF_A_Q);
>> +    fifo8_create(&s->tx_fifo, TXFF_A_Q);
>>      memory_region_init_io(&s->mmlqspi, &lqspi_ops, s, "lqspi",
>>                            (1 << LQSPI_ADDRESS_BITS) * 2);
>>      sysbus_init_mmio(sbd, &s->mmlqspi);
>
> Destroying and recreating the fifos seems a bit odd -- can you
> structure this so the base class instance init just creates them
> at the right size? I guess the obvious way would be to have the
> class struct have a field for fifo size which the class init
> function sets appropriately, and then instance init creates
> a fifo of that size.

Done.

Regards,
Peter

>This is kind of like how target-arm/cpu.c
> handles ID register fields, but in the class rather than the
> instance struct.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 07/15] xilinx_spips: Trash LQ page cache on mode change

[Peter Crosthwaite]

Hi Peter,

On Sat, Apr 6, 2013 at 4:53 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Invalidate the LQSPI cached page when transitioning into LQSPI mode.
>> Otherwise there is a possibility that the controller will return stale
>> data to the guest when transitioning back to LQ_MODE after a page
>> program.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index 78a3fec..4f921f8 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -390,6 +390,9 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>>      int mask = ~0;
>>      int man_start_com = 0;
>>      XilinxSPIPS *s = opaque;
>> +    /* FIXME: abstract away somehow */
>> +    XilinxQSPIPS *q = (XilinxQSPIPS *)object_dynamic_cast(OBJECT(s),
>> +                       TYPE_XILINX_QSPIPS);
>>
>
> Hmm, a C cast of the result of an object_dynamic_cast()?
> Would be nice to do this some other way... Maybe a member
> function (ie fn ptr in the class struct) which does nothing
> on SPIPS and clears the cached page on QSPIPS?
>

Went a different route. Added the MemoryRegionOps to the class instead
and switched that out on class init. qspis alternate write handler
just calls spis then does its side effects on top. This way, spi has
no awareness of qspi which is more ideal the child class specific
abstract fns in the base class def.

Regards,
Peter

>>      DB_PRINT("addr=" TARGET_FMT_plx " = %x\n", addr, (unsigned)value);
>>      addr >>= 2;
>> @@ -435,6 +438,11 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>>      case R_TXD3:
>>          tx_data_bytes(s, (uint32_t)value, 3);
>>          goto no_reg_update;
>> +    case R_LQSPI_CFG:
>> +        if (q) {
>> +            q->lqspi_cached_addr = ~0ULL;
>> +        }
>> +        break;
>>      }
>>      s->regs[addr] = (s->regs[addr] & ~mask) | (value & mask);
>>  no_reg_update:
>> --
>> 1.7.0.4
>>
>
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 11/15] xilinx_spips: Fix striping behaviour

[Peter Crosthwaite]

Hi Peter,

On Sat, Apr 6, 2013 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> The QSPI controller was using byte-wide stripes when striping across
>> the two flashes in dual parallel mode. The real hardware however uses
>> individual bit striping. QEMU misbehaves in the (corner) case where
>> data is written/read in dual-parallel mode and read/written back in
>> single mode.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |   74 ++++++++++++++++++++++++++++++++++++++---------------
>>  1 files changed, 53 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index a2019e4..43ce2d8 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -258,35 +258,67 @@ static void xilinx_spips_reset(DeviceState *d)
>>      xilinx_spips_update_cs_lines(s);
>>  }
>>
>> +static inline void stripe8(uint8_t *x, int num, bool dir)
>> +{
>> +    uint8_t r[num];
>> +    memset(r, 0, sizeof(uint8_t) * num);
>
> Don't interleave code and variable definitions, please.
>
>> +    int idx[2] = {0, 0};
>> +    int bit[2] = {0, 0};
>> +    int d = dir;
>> +
>> +    for (idx[0] = 0; idx[0] < num; ++idx[0]) {
>> +        for (bit[0] = 0; bit[0] < 8; ++bit[0]) {
>> +            r[idx[d]] |= x[idx[!d]] & 1 << bit[!d] ? 1 << bit[d] : 0;
>> +            idx[1] = (idx[1] + 1) % num;
>> +            if (!idx[1]) {
>> +                bit[1]++;
>> +            }
>> +        }
>> +    }
>> +    memcpy(x, r, sizeof(uint8_t) * num);
>> +}
>
> This function could really use a comment saying what it's doing...
>

+/* N way (num) in place bit striper. Lay out row wise bits (LSB to MSB)
+ * column wise (from element 0 to N-1). num is the length of x, and dir
+ * reverses the direction of the transform. Best illustrated by example:
+ * Each digit in the below array is a single bit (num == 3):
+ *
+ * {{ 76543210, }  ----- stripe (dir == false) -----> {{ FCheb630, }
+ *  { hgfedcba, }                                      { GDAfc741, }
+ *  { HGFEDCBA, }} <---- upstripe (dir == true) -----  { HEBgda52, }}
+ */
+

Regards,
Peter

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 15/15] xilinx_spips: lqspi: Fix byte/misaligned access

[Peter Crosthwaite]

On Sat, Apr 6, 2013 at 5:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> The LQSPI bus attachment supports byte/halfword and misaligned
>> accesses. Fixed. Refactored the LQSPI cache to be byte-wise
>> instead of word wise accordingly.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |   31 +++++++++++++++++--------------
>>  1 files changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index 32d8db8..cb45a9c 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -160,7 +160,7 @@ typedef struct {
>>  typedef struct {
>>      XilinxSPIPS parent_obj;
>>
>> -    uint32_t lqspi_buf[LQSPI_CACHE_SIZE];
>> +    uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
>
> Is it really right that this buffer isn't in the vmstate,
> by the way?
>

Yes,

Its a software transparent cache. The cache is lost on migration. Post
load the first access just refreshes the cache and the guest is none
the wiser.

Regards.
{eter

>>      hwaddr lqspi_cached_addr;
>>  } XilinxQSPIPS;
>
> -- PMM
>

Re: [Qemu-devel] [PATCH arm-devs v1 10/15] xilinx_spips: Fix CTRL register RW bits

[Peter Crosthwaite]

Hi Peter,

On Sat, Apr 6, 2013 at 4:57 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> The CTRL register was RAZ/WI on some of the RW bits. Even though the
>> function behind these bits is invalid in QEMU, they should still be
>> guest accessible. Fix.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/xilinx_spips.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c
>> index 16c2e1d..a2019e4 100644
>> --- a/hw/xilinx_spips.c
>> +++ b/hw/xilinx_spips.c
>> @@ -341,7 +341,7 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
>>      addr >>= 2;
>>      switch (addr) {
>>      case R_CONFIG:
>> -        mask = 0x0002FFFF;
>> +        mask = 0x840AFFFF;
>>          break;
>>      case R_INTR_STATUS:
>>          ret = s->regs[addr] & IXR_ALL;
>> @@ -404,7 +404,7 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>>      addr >>= 2;
>>      switch (addr) {
>>      case R_CONFIG:
>> -        mask = 0x0002FFFF;
>> +        mask = 0x840AFFFF;
>>          if (value & MAN_START_COM) {
>>              man_start_com = 1;
>>          }
>> --
>> 1.7.0.4
>>
>
>
> Maybe the magic number for 'implemented bits in the register'
> deserves a #define ?
>

Took the reverse approach, and defined the unimplemented (reserved)
bits and inverted to make mask.

Regards,
Peter

> -- PMM
>