[Qemu-devel] [PATCH 00/17] Initial i.MX7 support

[Qemu-devel] [PATCH 00/17] Initial i.MX7 support

[Andrey Smirnov]

Hi everyone,

This patch series contains the work that I've done in order to enable
support for i.MX7 emulation in QEMU. Majority of the set are just odd
fixes and small features implementation that I had to do to already
exisitng code but last 5 commits contain new emulation code.

As the one before last commit in the series states the supported i.MX7
features are:

    * up to 2 Cortex A9 cores (SMP works with PSCI)
    * A7 MPCORE (identical to A15 MPCORE)
    * 7 i.MX UARTs
    * 1 CCM device
    * 2 Ethernet controllers (FEC)
    * 3 SD controllers (USDHC)
    * 1 SNVS device
    * 1 WDT device

I also have a follow up series that implements bit needes for PCIe
emulation support (DesignWare IP emulation + supporting code) which
I'll be submitting after this series is accepted.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (17):
  imx_fec: Do not link to netdev
  imx_fec: Do not calculate FEC
  imx_fec: Refactor imx_eth_enable_rx()
  imx_fec: Change queue flushing heuristics
  imx_fec: Use ENET_FTRL to determine truncation length
  imx_fec: Use MIN instead of explicit ternary operator
  imx_fec: Emulate SHIFT16 in ENETx_RACC
  imx_fec: Add support for multiple Tx DMA rings
  imx_fec: Use correct length for packet size
  sdhci: Add i.MX specific subtype of SDHCI
  sdhci: Implement write method of ACMD12ERRSTS register
  i.MX: Add i.MX7 CCM, PMU and ANALOG device
  i.MX: Add code to emulate i.MX2 watchdog IP block
  i.MX7: Add code to emulate SNVS IP-block
  include/qemu: Add sizes.h from Linux
  i.MX: Add i.MX7 SOC implementation.
  Implement support for i.MX7 Sabre board

 default-configs/arm-softmmu.mak |   1 +
 hw/arm/Makefile.objs            |   2 +
 hw/arm/fsl-imx6.c               |   1 +
 hw/arm/fsl-imx7.c               | 327 ++++++++++++++++++++++++++++++++++++++++
 hw/arm/mcimx7d-sabre.c          | 100 ++++++++++++
 hw/misc/Makefile.objs           |   3 +
 hw/misc/imx2_wdt.c              | 117 ++++++++++++++
 hw/misc/imx7_ccm.c              | 201 ++++++++++++++++++++++++
 hw/misc/imx7_snvs.c             |  84 +++++++++++
 hw/net/imx_fec.c                | 153 ++++++++++++++-----
 hw/sd/sdhci-internal.h          |  15 ++
 hw/sd/sdhci.c                   | 126 +++++++++++++++-
 include/hw/arm/fsl-imx7.h       | 114 ++++++++++++++
 include/hw/misc/imx2_wdt.h      |  36 +++++
 include/hw/misc/imx7_ccm.h      |  76 ++++++++++
 include/hw/misc/imx7_snvs.h     |  35 +++++
 include/hw/net/imx_fec.h        |  26 +++-
 include/hw/sd/sdhci.h           |   8 +
 include/qemu/sizes.h            |  47 ++++++
 19 files changed, 1433 insertions(+), 39 deletions(-)
 create mode 100644 hw/arm/fsl-imx7.c
 create mode 100644 hw/arm/mcimx7d-sabre.c
 create mode 100644 hw/misc/imx2_wdt.c
 create mode 100644 hw/misc/imx7_ccm.c
 create mode 100644 hw/misc/imx7_snvs.c
 create mode 100644 include/hw/arm/fsl-imx7.h
 create mode 100644 include/hw/misc/imx2_wdt.h
 create mode 100644 include/hw/misc/imx7_ccm.h
 create mode 100644 include/hw/misc/imx7_snvs.h
 create mode 100644 include/qemu/sizes.h

-- 
2.13.5

[Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev

[Andrey Smirnov]

Binding to a particular netdev doesn't seem to belong to this layer
and should probably be done as a part of board or SoC specific code.

Convert all of the users of this IP block to use
qdev_set_nic_properties() instead.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/arm/fsl-imx6.c | 1 +
 hw/net/imx_fec.c  | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 26fd214004..2ed7146c52 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -385,6 +385,7 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                             spi_table[i].irq));
     }
 
+    qdev_set_nic_properties(DEVICE(&s->eth), &nd_table[0]);
     object_property_set_bool(OBJECT(&s->eth), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 90e6ee35ba..88b4b049d7 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1171,8 +1171,6 @@ static void imx_eth_realize(DeviceState *dev, Error **errp)
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
 
-    s->conf.peers.ncs[0] = nd_table[0].netdev;
-
     s->nic = qemu_new_nic(&imx_eth_net_info, &s->conf,
                           object_get_typename(OBJECT(dev)),
                           DEVICE(dev)->id, s);
-- 
2.13.5

[Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC

[Andrey Smirnov]

Save some computation time and avoid calculating CRC's frame

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 88b4b049d7..75822344fc 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1032,9 +1032,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
     IMXENETBufDesc bd;
     uint32_t flags = 0;
     uint32_t addr;
-    uint32_t crc;
     uint32_t buf_addr;
-    uint8_t *crc_ptr;
     unsigned int buf_len;
     size_t size = len;
 
@@ -1048,8 +1046,6 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
 
     /* 4 bytes for the CRC.  */
     size += 4;
-    crc = cpu_to_be32(crc32(~0, buf, size));
-    crc_ptr = (uint8_t *) &crc;
 
     /* Huge frames are truncted.  */
     if (size > ENET_MAX_FRAME_SIZE) {
@@ -1090,9 +1086,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
         dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
         buf += buf_len;
         if (size < 4) {
+            const uint8_t zeros[4] = { 0 };
+
             dma_memory_write(&address_space_memory, buf_addr + buf_len,
-                             crc_ptr, 4 - size);
-            crc_ptr += 4 - size;
+                             zeros, 4 - size);
         }
         bd.flags &= ~ENET_BD_E;
         if (size == 0) {
-- 
2.13.5

[Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx()

[Andrey Smirnov]

Refactor imx_eth_enable_rx() to have more meaningfull variable name
than 'tmp' and to reduce number of logical negations done.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 75822344fc..84085afe09 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -536,19 +536,19 @@ static void imx_eth_do_tx(IMXFECState *s)
 static void imx_eth_enable_rx(IMXFECState *s)
 {
     IMXFECBufDesc bd;
-    bool tmp;
+    bool rx_ring_full;
 
     imx_fec_read_bd(&bd, s->rx_descriptor);
 
-    tmp = ((bd.flags & ENET_BD_E) != 0);
+    rx_ring_full = !(bd.flags & ENET_BD_E);
 
-    if (!tmp) {
+    if (rx_ring_full) {
         FEC_PRINTF("RX buffer full\n");
     } else if (!s->regs[ENET_RDAR]) {
         qemu_flush_queued_packets(qemu_get_queue(s->nic));
     }
 
-    s->regs[ENET_RDAR] = tmp ? ENET_RDAR_RDAR : 0;
+    s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
 }
 
 static void imx_eth_reset(DeviceState *d)
-- 
2.13.5

[Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics

[Andrey Smirnov]

In current implementation, packet queue flushing logic seem to suffer
from a deadlock like scenario if a packet is received by the interface
before before Rx ring is initialized by Guest's driver. Consider the
following sequence of events:

	1. A QEMU instance is started against a TAP device on Linux
	   host, running Linux guest, e. g., something to the effect
	   of:

	   qemu-system-arm \
	      -net nic,model=imx.fec,netdev=lan0 \
	      netdev tap,id=lan0,ifname=tap0,script=no,downscript=no \
	      ... rest of the arguments ...

	2. Once QEMU starts, but before guest reaches the point where
	   FEC deriver is done initializing the HW, Guest, via TAP
	   interface, receives a number of multicast MDNS packets from
	   Host (not necessarily true for every OS, but it happens at
	   least on Fedora 25)

	3. Recieving a packet in such a state results in
	   imx_eth_can_receive() returning '0', which in turn causes
	   tap_send() to disable corresponding event (tap.c:203)

	4. Once Guest's driver reaches the point where it is ready to
	   recieve packets it prepares Rx ring descriptors and writes
	   ENET_RDAR_RDAR to ENET_RDAR register to indicate to HW that
	   more descriptors are ready. And at this points emulation
	   layer does this:

	   	 s->regs[index] = ENET_RDAR_RDAR;
                 imx_eth_enable_rx(s);

	   which, combined with:

	   	  if (!s->regs[ENET_RDAR]) {
		     qemu_flush_queued_packets(qemu_get_queue(s->nic));
		  }

	   results in Rx queue never being flushed and corresponding
	   I/O event beign disabled.

Change the code to remember the fact that can_receive callback was
called before Rx ring was ready and use it to make a decision if
receive queue needs to be flushed.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c         | 6 ++++--
 include/hw/net/imx_fec.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 84085afe09..767402909d 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -544,8 +544,9 @@ static void imx_eth_enable_rx(IMXFECState *s)
 
     if (rx_ring_full) {
         FEC_PRINTF("RX buffer full\n");
-    } else if (!s->regs[ENET_RDAR]) {
+    } else if (s->needs_flush) {
         qemu_flush_queued_packets(qemu_get_queue(s->nic));
+        s->needs_flush = false;
     }
 
     s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
@@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
 
     FEC_PRINTF("\n");
 
-    return s->regs[ENET_RDAR] ? 1 : 0;
+    s->needs_flush = !s->regs[ENET_RDAR];
+    return !!s->regs[ENET_RDAR];
 }
 
 static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 62ad473b05..4bc8f03ec2 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -252,6 +252,7 @@ typedef struct IMXFECState {
     uint32_t phy_int_mask;
 
     bool is_fec;
+    bool needs_flush;
 } IMXFECState;
 
 #endif
-- 
2.13.5

[Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length

[Andrey Smirnov]

Frame truncation length, TRUNC_FL, is determined by the contents of
ENET_FTRL register, so convert the code to use it instead of a
hardcoded constant.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 767402909d..989c11be5f 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
     size += 4;
 
     /* Huge frames are truncted.  */
-    if (size > ENET_MAX_FRAME_SIZE) {
-        size = ENET_MAX_FRAME_SIZE;
+    if (size > s->regs[ENET_FTRL]) {
+        size = s->regs[ENET_FTRL];
         flags |= ENET_BD_TR | ENET_BD_LG;
     }
 
-- 
2.13.5

[Qemu-devel] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator

[Andrey Smirnov]

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 989c11be5f..8a77136d38 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1074,7 +1074,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
                           TYPE_IMX_FEC, __func__);
             break;
         }
-        buf_len = (size <= s->regs[ENET_MRBR]) ? size : s->regs[ENET_MRBR];
+        buf_len = MIN(size, s->regs[ENET_MRBR]);
         bd.length = buf_len;
         size -= buf_len;
 
-- 
2.13.5

[Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC

[Andrey Smirnov]

Needed to support latest Linux kernel driver which relies on that
functionality.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c         | 23 +++++++++++++++++++++++
 include/hw/net/imx_fec.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 8a77136d38..bd62d7a75f 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -1037,6 +1037,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
     uint32_t buf_addr;
     unsigned int buf_len;
     size_t size = len;
+    bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
 
     FEC_PRINTF("len %d\n", (int)size);
 
@@ -1049,6 +1050,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
     /* 4 bytes for the CRC.  */
     size += 4;
 
+    if (shift16) {
+        size += 2;
+    }
+
     /* Huge frames are truncted.  */
     if (size > s->regs[ENET_FTRL]) {
         size = s->regs[ENET_FTRL];
@@ -1085,6 +1090,24 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
             buf_len += size - 4;
         }
         buf_addr = bd.data;
+
+        if (shift16) {
+            /*
+             * If SHIFT16 bit of ENETx_RACC register is set we need to
+             * align the payload to 4-byte boundary.
+             */
+            const uint8_t zeros[2] = { 0 };
+
+            dma_memory_write(&address_space_memory, buf_addr,
+                             zeros, sizeof(zeros));
+
+            buf_addr += sizeof(zeros);
+            buf_len  -= sizeof(zeros);
+
+            shift16 = false; /* We only do this once per Ethernet
+                              * frame */
+        }
+
         dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
         buf += buf_len;
         if (size < 4) {
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 4bc8f03ec2..20a6aa98b4 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -169,6 +169,8 @@
 #define ENET_TWFR_TFWR_LENGTH  (6)
 #define ENET_TWFR_STRFWD       (1 << 8)
 
+#define ENET_RACC_SHIFT16	BIT(7)
+
 /* Buffer Descriptor.  */
 typedef struct {
     uint16_t length;
-- 
2.13.5

[Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings

[Andrey Smirnov]

More recent version of the IP block support more than one Tx DMA ring,
so add the code implementing that feature.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c         | 97 +++++++++++++++++++++++++++++++++++++++---------
 include/hw/net/imx_fec.h | 23 +++++++++++-
 2 files changed, 101 insertions(+), 19 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index bd62d7a75f..6045ffe673 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -196,6 +196,17 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
     }
 }
 
+static const VMStateDescription vmstate_imx_eth_tx_ring = {
+    .name = "fec-tx-ring",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(descriptor, IMXFECTxRing),
+        VMSTATE_UINT32(tdsr, IMXFECTxRing),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_imx_eth = {
     .name = TYPE_IMX_FEC,
     .version_id = 2,
@@ -203,8 +214,10 @@ static const VMStateDescription vmstate_imx_eth = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, IMXFECState, ENET_MAX),
         VMSTATE_UINT32(rx_descriptor, IMXFECState),
-        VMSTATE_UINT32(tx_descriptor, IMXFECState),
-
+        VMSTATE_STRUCT_ARRAY(tx_ring, IMXFECState,
+                             ENET_TX_RING_NUM,
+                             1, vmstate_imx_eth_tx_ring,
+                             IMXFECTxRing),
         VMSTATE_UINT32(phy_status, IMXFECState),
         VMSTATE_UINT32(phy_control, IMXFECState),
         VMSTATE_UINT32(phy_advertise, IMXFECState),
@@ -407,7 +420,7 @@ static void imx_fec_do_tx(IMXFECState *s)
     int frame_size = 0, descnt = 0;
     uint8_t frame[ENET_MAX_FRAME_SIZE];
     uint8_t *ptr = frame;
-    uint32_t addr = s->tx_descriptor;
+    uint32_t addr = s->tx_ring[0].descriptor;
 
     while (descnt++ < IMX_MAX_DESC) {
         IMXFECBufDesc bd;
@@ -448,17 +461,38 @@ static void imx_fec_do_tx(IMXFECState *s)
         }
     }
 
-    s->tx_descriptor = addr;
+    s->tx_ring[0].descriptor = addr;
 
     imx_eth_update(s);
 }
 
-static void imx_enet_do_tx(IMXFECState *s)
+static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
 {
     int frame_size = 0, descnt = 0;
     uint8_t frame[ENET_MAX_FRAME_SIZE];
     uint8_t *ptr = frame;
-    uint32_t addr = s->tx_descriptor;
+    IMXFECTxRing *ring;
+    uint32_t addr;
+
+    switch (index) {
+    case ENET_TDAR:
+        ring = &s->tx_ring[0];
+        break;
+    case ENET_TDAR1:
+        ring = &s->tx_ring[1];
+        break;
+    case ENET_TDAR2:
+        ring = &s->tx_ring[2];
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: bogus value for index %x\n",
+                      __func__, index);
+        abort();
+        break;
+    }
+
+    addr = ring->descriptor;
 
     while (descnt++ < IMX_MAX_DESC) {
         IMXENETBufDesc bd;
@@ -502,32 +536,32 @@ static void imx_enet_do_tx(IMXFECState *s)
             ptr = frame;
             frame_size = 0;
             if (bd.option & ENET_BD_TX_INT) {
-                s->regs[ENET_EIR] |= ENET_INT_TXF;
+                s->regs[ENET_EIR] |= ring->int_txf;
             }
         }
         if (bd.option & ENET_BD_TX_INT) {
-            s->regs[ENET_EIR] |= ENET_INT_TXB;
+            s->regs[ENET_EIR] |= ring->int_txb;
         }
         bd.flags &= ~ENET_BD_R;
         /* Write back the modified descriptor.  */
         imx_enet_write_bd(&bd, addr);
         /* Advance to the next descriptor.  */
         if ((bd.flags & ENET_BD_W) != 0) {
-            addr = s->regs[ENET_TDSR];
+            addr = s->regs[ring->tdsr];
         } else {
             addr += sizeof(bd);
         }
     }
 
-    s->tx_descriptor = addr;
+    ring->descriptor = addr;
 
     imx_eth_update(s);
 }
 
-static void imx_eth_do_tx(IMXFECState *s)
+static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
 {
     if (!s->is_fec && (s->regs[ENET_ECR] & ENET_ECR_EN1588)) {
-        imx_enet_do_tx(s);
+        imx_enet_do_tx(s, index);
     } else {
         imx_fec_do_tx(s);
     }
@@ -586,7 +620,22 @@ static void imx_eth_reset(DeviceState *d)
     }
 
     s->rx_descriptor = 0;
-    s->tx_descriptor = 0;
+
+    s->tx_ring[0].tdsr = ENET_TDSR;
+    s->tx_ring[1].tdsr = ENET_TDSR1;
+    s->tx_ring[2].tdsr = ENET_TDSR2;
+
+    s->tx_ring[0].int_txf = ENET_INT_TXF;
+    s->tx_ring[1].int_txf = ENET_INT_TXF1;
+    s->tx_ring[2].int_txf = ENET_INT_TXF2;
+
+    s->tx_ring[0].int_txb = ENET_INT_TXB;
+    s->tx_ring[1].int_txb = ENET_INT_TXB1;
+    s->tx_ring[2].int_txb = ENET_INT_TXB2;
+
+    s->tx_ring[0].descriptor = 0;
+    s->tx_ring[1].descriptor = 0;
+    s->tx_ring[2].descriptor = 0;
 
     /* We also reset the PHY */
     phy_reset(s);
@@ -814,10 +863,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
             s->regs[index] = 0;
         }
         break;
+    case ENET_TDAR1:    /* FALLTHROUGH */
+    case ENET_TDAR2:    /* FALLTHROUGH */
     case ENET_TDAR:
         if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
             s->regs[index] = ENET_TDAR_TDAR;
-            imx_eth_do_tx(s);
+            imx_eth_do_tx(s, index);
         }
         s->regs[index] = 0;
         break;
@@ -829,8 +880,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
         if ((s->regs[index] & ENET_ECR_ETHEREN) == 0) {
             s->regs[ENET_RDAR] = 0;
             s->rx_descriptor = s->regs[ENET_RDSR];
-            s->regs[ENET_TDAR] = 0;
-            s->tx_descriptor = s->regs[ENET_TDSR];
+            s->regs[ENET_TDAR]  = 0;
+            s->regs[ENET_TDAR1] = 0;
+            s->regs[ENET_TDAR2] = 0;
+            s->tx_ring[0].descriptor = s->regs[ENET_TDSR];
+            s->tx_ring[1].descriptor = s->regs[ENET_TDSR1];
+            s->tx_ring[2].descriptor = s->regs[ENET_TDSR2];
         }
         break;
     case ENET_MMFR:
@@ -908,7 +963,15 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
         } else {
             s->regs[index] = value & ~7;
         }
-        s->tx_descriptor = s->regs[index];
+        s->tx_ring[0].descriptor = s->regs[index];
+        break;
+    case ENET_TDSR1:
+        s->regs[index] = value & ~7;
+        s->tx_ring[1].descriptor = s->regs[index];
+        break;
+    case ENET_TDSR2:
+        s->regs[index] = value & ~7;
+        s->tx_ring[2].descriptor = s->regs[index];
         break;
     case ENET_MRBR:
         s->regs[index] = value & 0x00003ff0;
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index 20a6aa98b4..40bd29771f 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -52,6 +52,8 @@
 #define ENET_TFWR              81
 #define ENET_FRBR              83
 #define ENET_FRSR              84
+#define ENET_TDSR1             89
+#define ENET_TDSR2             92
 #define ENET_RDSR              96
 #define ENET_TDSR              97
 #define ENET_MRBR              98
@@ -66,6 +68,8 @@
 #define ENET_FTRL              108
 #define ENET_TACC              112
 #define ENET_RACC              113
+#define ENET_TDAR1             121
+#define ENET_TDAR2             123
 #define ENET_MIIGSK_CFGR       192
 #define ENET_MIIGSK_ENR        194
 #define ENET_ATCR              256
@@ -106,13 +110,18 @@
 #define ENET_INT_WAKEUP        (1 << 17)
 #define ENET_INT_TS_AVAIL      (1 << 16)
 #define ENET_INT_TS_TIMER      (1 << 15)
+#define ENET_INT_TXF2          (1 <<  7)
+#define ENET_INT_TXB2          (1 <<  6)
+#define ENET_INT_TXF1          (1 <<  3)
+#define ENET_INT_TXB1          (1 <<  2)
 
 #define ENET_INT_MAC           (ENET_INT_HB | ENET_INT_BABR | ENET_INT_BABT | \
                                 ENET_INT_GRA | ENET_INT_TXF | ENET_INT_TXB | \
                                 ENET_INT_RXF | ENET_INT_RXB | ENET_INT_MII | \
                                 ENET_INT_EBERR | ENET_INT_LC | ENET_INT_RL | \
                                 ENET_INT_UN | ENET_INT_PLR | ENET_INT_WAKEUP | \
-                                ENET_INT_TS_AVAIL)
+                                ENET_INT_TS_AVAIL | ENET_INT_TXF1 | ENET_INT_TXB1 |\
+                                ENET_INT_TXF2 | ENET_INT_TXB2)
 
 /* RDAR */
 #define ENET_RDAR_RDAR         (1 << 24)
@@ -233,6 +242,15 @@ typedef struct {
 
 #define ENET_BD_BDU            (1 << 31)
 
+#define ENET_TX_RING_NUM       3
+
+typedef struct IMXFECTxRing {
+    uint32_t descriptor;
+    uint32_t tdsr;
+    uint32_t int_txf;
+    uint32_t int_txb;
+} IMXFECTxRing;
+
 typedef struct IMXFECState {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -245,7 +263,8 @@ typedef struct IMXFECState {
 
     uint32_t regs[ENET_MAX];
     uint32_t rx_descriptor;
-    uint32_t tx_descriptor;
+
+    IMXFECTxRing tx_ring[ENET_TX_RING_NUM];
 
     uint32_t phy_status;
     uint32_t phy_control;
-- 
2.13.5

[Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size

[Andrey Smirnov]

Use 'frame_size' instead of 'len' when calling qemu_send_packet(),
failing to do so results in malformed packets send in case when that
packed is fragmented into multiple DMA transactions.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/net/imx_fec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 6045ffe673..c45b9648d9 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -532,7 +532,7 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
                 }
             }
             /* Last buffer in frame.  */
-            qemu_send_packet(qemu_get_queue(s->nic), frame, len);
+            qemu_send_packet(qemu_get_queue(s->nic), frame, frame_size);
             ptr = frame;
             frame_size = 0;
             if (bd.option & ENET_BD_TX_INT) {
-- 
2.13.5

[Qemu-devel] [PATCH 10/17] sdhci: Add i.MX specific subtype of SDHCI

[Andrey Smirnov]

IP block found on several generations of i.MX family does not use
vanilla SDHCI implementation and it comes with a number of quirks.

Introduce i.MX SDHCI subtype of SDHCI block to add code necessary to
support unmodified Linux guest driver.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/sd/sdhci-internal.h |  15 ++++++
 hw/sd/sdhci.c          | 123 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/sd/sdhci.h  |   8 ++++
 3 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 161177cf39..7b9ed06c36 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -91,6 +91,8 @@
 #define SDHC_CTRL_ADMA2_32             0x10
 #define SDHC_CTRL_ADMA2_64             0x18
 #define SDHC_DMA_TYPE(x)               ((x) & SDHC_CTRL_DMA_CHECK_MASK)
+#define SDHC_CTRL_4BITBUS              0x02
+#define SDHC_CTRL_8BITBUS              0x20
 
 /* R/W Power Control Register 0x0 */
 #define SDHC_PWRCON                    0x29
@@ -229,4 +231,17 @@ enum {
 
 extern const VMStateDescription sdhci_vmstate;
 
+
+#define ESDHC_MIX_CTRL			0x48
+#define ESDHC_VENDOR_SPEC		0xc0
+#define ESDHC_DLL_CTRL			0x60
+
+#define ESDHC_TUNING_CTRL		0xcc
+#define ESDHC_TUNE_CTRL_STATUS		0x68
+#define ESDHC_WTMK_LVL			0x44
+
+#define ESDHC_CTRL_4BITBUS		(0x1 << 1)
+#define ESDHC_CTRL_8BITBUS		(0x2 << 1)
+
+
 #endif
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6d6a791ee9..73e7910ba9 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -265,7 +265,8 @@ static void sdhci_send_command(SDHCIState *s)
             }
         }
 
-        if ((s->norintstsen & SDHC_NISEN_TRSCMP) &&
+        if (!(s->quirks & SDHCI_QUIRK_NO_BUSY_IRQ) &&
+            (s->norintstsen & SDHC_NISEN_TRSCMP) &&
             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
             s->norintsts |= SDHC_NIS_TRSCMP;
         }
@@ -1191,6 +1192,8 @@ static void sdhci_initfn(SDHCIState *s)
 
     s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
     s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
+
+    s->io_ops = &sdhci_mmio_ops;
 }
 
 static void sdhci_uninitfn(SDHCIState *s)
@@ -1347,7 +1350,7 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
     sysbus_init_irq(sbd, &s->irq);
-    memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
+    memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
             SDHC_REGISTERS_MAP_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
 }
@@ -1386,11 +1389,127 @@ static const TypeInfo sdhci_bus_info = {
     .class_init = sdhci_bus_class_init,
 };
 
+static uint64_t usdhc_read(void *opaque, hwaddr offset, unsigned size)
+{
+    SDHCIState *s = SYSBUS_SDHCI(opaque);
+    uint32_t ret;
+    uint16_t hostctl;
+
+    switch (offset) {
+    default:
+        return sdhci_read(opaque, offset, size);
+
+    case SDHC_HOSTCTL:
+        hostctl = SDHC_DMA_TYPE(s->hostctl) << 5;
+
+        if (s->hostctl & SDHC_CTRL_8BITBUS)
+            hostctl |= ESDHC_CTRL_8BITBUS;
+
+        if (s->hostctl & SDHC_CTRL_4BITBUS)
+            hostctl |= ESDHC_CTRL_4BITBUS;
+
+        ret = hostctl | (s->blkgap << 16) |
+            (s->wakcon << 24);
+
+        break;
+
+    case ESDHC_DLL_CTRL:
+    case ESDHC_TUNE_CTRL_STATUS:
+    case 0x6c:
+    case ESDHC_TUNING_CTRL:
+    case ESDHC_VENDOR_SPEC:
+    case ESDHC_MIX_CTRL:
+    case ESDHC_WTMK_LVL:
+        ret = 0;
+        break;
+    }
+
+    return ret;
+}
+
+static void
+usdhc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
+{
+    SDHCIState *s = SYSBUS_SDHCI(opaque);
+    uint8_t hostctl = 0;
+    uint32_t value = (uint32_t)val;
+
+    switch (offset) {
+    case ESDHC_DLL_CTRL:
+    case ESDHC_TUNE_CTRL_STATUS:
+    case 0x6c:
+    case ESDHC_TUNING_CTRL:
+    case ESDHC_WTMK_LVL:
+    case ESDHC_VENDOR_SPEC:
+        break;
+
+    case SDHC_HOSTCTL:
+        if (value & ESDHC_CTRL_8BITBUS)
+            hostctl |= SDHC_CTRL_8BITBUS;
+
+        if (value & ESDHC_CTRL_4BITBUS)
+            hostctl |= ESDHC_CTRL_4BITBUS;
+
+        hostctl |= SDHC_DMA_TYPE(value >> 5);
+
+        value &= ~0xFE;
+        value |= hostctl;
+        value &= ~0xFF00;
+        value |= s->pwrcon;
+
+        sdhci_write(opaque, offset, value, size);
+        break;
+
+    case ESDHC_MIX_CTRL:
+        /*
+         * The layout of the register is slightly different, but we
+         * don't care about those bits
+         */
+        s->trnmod = value & 0xFFFF;
+        break;
+    case SDHC_TRNMOD:
+        sdhci_write(opaque, offset, val | s->trnmod, size);
+        break;
+    case SDHC_BLKSIZE:
+        val |= 0x7 << 12;
+    default:                    /* FALLTHROUGH */
+        sdhci_write(opaque, offset, val, size);
+        break;
+    }
+}
+
+
+static const MemoryRegionOps usdhc_mmio_ops = {
+    .read = usdhc_read,
+    .write = usdhc_write,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+        .unaligned = false
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void imx_usdhc_init(Object *obj)
+{
+    SDHCIState *s = SYSBUS_SDHCI(obj);
+
+    s->io_ops = &usdhc_mmio_ops;
+    s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
+}
+
+static const TypeInfo imx_usdhc_info = {
+    .name = TYPE_IMX_USDHC,
+    .parent = TYPE_SYSBUS_SDHCI,
+    .instance_init = imx_usdhc_init,
+};
+
 static void sdhci_register_types(void)
 {
     type_register_static(&sdhci_pci_info);
     type_register_static(&sdhci_sysbus_info);
     type_register_static(&sdhci_bus_info);
+    type_register_static(&imx_usdhc_info);
 }
 
 type_init(sdhci_register_types)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 0f0c3f1e64..903472c901 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -39,6 +39,7 @@ typedef struct SDHCIState {
     };
     SDBus sdbus;
     MemoryRegion iomem;
+    const MemoryRegionOps *io_ops;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
@@ -83,8 +84,13 @@ typedef struct SDHCIState {
     /* Force Event Auto CMD12 Error Interrupt Reg - write only */
     /* Force Event Error Interrupt Register- write only */
     /* RO Host Controller Version Register always reads as 0x2401 */
+
+    unsigned long quirks;
 } SDHCIState;
 
+/* Controller does not provide transfer-complete interrupt when not busy */
+#define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<14)
+
 #define TYPE_PCI_SDHCI "sdhci-pci"
 #define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj), TYPE_PCI_SDHCI)
 
@@ -92,4 +98,6 @@ typedef struct SDHCIState {
 #define SYSBUS_SDHCI(obj)                               \
      OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
 
+#define TYPE_IMX_USDHC "imx-usdhc"
+
 #endif /* SDHCI_H */
-- 
2.13.5

[Qemu-devel] [PATCH 11/17] sdhci: Implement write method of ACMD12ERRSTS register

[Andrey Smirnov]

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/sd/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 73e7910ba9..9249471957 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1139,6 +1139,9 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
                 ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
         break;
+    case SDHC_ACMD12ERRSTS:
+        MASKED_WRITE(s->acmd12errsts, mask, value);
+        break;
     case SDHC_FEAER:
         s->acmd12errsts |= value;
         s->errintsts |= (value >> 16) & s->errintstsen;
-- 
2.13.5

[Qemu-devel] [PATCH 12/17] i.MX: Add i.MX7 CCM, PMU and ANALOG device

[Andrey Smirnov]

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/misc/Makefile.objs      |   1 +
 hw/misc/imx7_ccm.c         | 201 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/imx7_ccm.h |  76 +++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 hw/misc/imx7_ccm.c
 create mode 100644 include/hw/misc/imx7_ccm.h

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 29fb922cef..ac1be05a03 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -34,6 +34,7 @@ obj-$(CONFIG_IMX) += imx31_ccm.o
 obj-$(CONFIG_IMX) += imx25_ccm.o
 obj-$(CONFIG_IMX) += imx6_ccm.o
 obj-$(CONFIG_IMX) += imx6_src.o
+obj-$(CONFIG_IMX) += imx7_ccm.o
 obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
 obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
 obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx7_ccm.c b/hw/misc/imx7_ccm.c
new file mode 100644
index 0000000000..418aafe7cc
--- /dev/null
+++ b/hw/misc/imx7_ccm.c
@@ -0,0 +1,201 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 CCM, PMU and ANALOG IP blocks emulation code
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * 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 "qemu/osdep.h"
+#include "qemu/sizes.h"
+#include "hw/misc/imx7_ccm.h"
+#include "qemu/log.h"
+
+static uint32_t imx7_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)
+{
+    /*
+     * This function is "consumed" by GPT emulation code, however on
+     * i.MX7 each GPT block can have their own clock root. This means
+     * that this functions needs somehow to know requester's identity
+     * and the way to pass it: be it via additional IMXClk constants
+     * or by adding another argument to this method needs to be
+     * figured out
+     */
+    qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Not implemented\n",
+                  TYPE_IMX7_CCM, __func__);
+    return 0;
+}
+
+static void imx7_ccm_reset(DeviceState *dev)
+{
+    IMX7CCMState *s = IMX7_CCM(dev);
+
+    s->analog[CCM_ANALOG_PLL_ARM]         = 0x00002042;
+    s->analog[CCM_ANALOG_PLL_DDR]         = 0x0060302c;
+    s->analog[CCM_ANALOG_PLL_DDR_SS]      = 0x00000000;
+    s->analog[CCM_ANALOG_PLL_DDR_NUM]     = 0x06aaac4d;
+    s->analog[CCM_ANALOG_PLL_DDR_DENOM]   = 0x100003ec;
+    s->analog[CCM_ANALOG_PLL_480]         = 0x00002000;    
+    s->analog[CCM_ANALOG_PLL_480A]        = 0x52605a56;
+    s->analog[CCM_ANALOG_PLL_480B]        = 0x52525216;
+    s->analog[CCM_ANALOG_PLL_ENET]        = 0x00001fc0;
+    s->analog[CCM_ANALOG_PLL_AUDIO]       = 0x0001301b;
+    s->analog[CCM_ANALOG_PLL_AUDIO_SS]    = 0x00000000;
+    s->analog[CCM_ANALOG_PLL_AUDIO_NUM]   = 0x05f5e100;
+    s->analog[CCM_ANALOG_PLL_AUDIO_DENOM] = 0x2964619c;
+    s->analog[CCM_ANALOG_PLL_VIDEO]       = 0x0008201b;
+    s->analog[CCM_ANALOG_PLL_VIDEO_SS]    = 0x00000000;
+    s->analog[CCM_ANALOG_PLL_VIDEO_NUM]   = 0x0000f699;
+    s->analog[CCM_ANALOG_PLL_VIDEO_DENOM] = 0x000f4240;
+    s->analog[CCM_ANALOG_PLL_MISC0]       = 0x00000000;
+
+    /* all PLLs need to be locked */
+    s->analog[CCM_ANALOG_PLL_ARM]   |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_DDR]   |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_480]   |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_480A]  |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_480B]  |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_ENET]  |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_AUDIO] |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_VIDEO] |= CCM_ANALOG_PLL_LOCK;
+    s->analog[CCM_ANALOG_PLL_MISC0] |= CCM_ANALOG_PLL_LOCK;
+}
+
+#define CCM_INDEX(offset)	(((offset) & ~(hwaddr)0xF) / sizeof(uint32_t))
+#define CCM_BITOP(offset)	((offset) & (hwaddr)0xF)
+
+enum {
+    CCM_BITOP_NONE = 0x00,
+    CCM_BITOP_SET  = 0x04,
+    CCM_BITOP_CLR  = 0x08,
+    CCM_BITOP_TOG  = 0x0C,
+};
+
+static uint64_t imx7_set_clr_tog_read(void *opaque, hwaddr offset,
+                                      unsigned size)
+{
+    const uint32_t *mmio = opaque;
+
+    return (uint64_t)mmio[CCM_INDEX(offset)];
+}
+
+static void imx7_set_clr_tog_write(void *opaque, hwaddr offset,
+                                   uint64_t value, unsigned size)
+{
+    const uint8_t  bitop = CCM_BITOP(offset);
+    const uint32_t index = CCM_INDEX(offset);
+    uint32_t *mmio = opaque;
+
+    switch (bitop) {
+    case CCM_BITOP_NONE:
+        mmio[index]  = value;
+        break;
+    case CCM_BITOP_SET:
+        mmio[index] |= value;
+        break;
+    case CCM_BITOP_CLR:
+        mmio[index] &= ~value;
+        break;
+    case CCM_BITOP_TOG:
+        mmio[index] ^= value;
+        break;
+    };
+}
+
+static const struct MemoryRegionOps imx7_set_clr_tog_ops = {
+    .read = imx7_set_clr_tog_read,
+    .write = imx7_set_clr_tog_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 imx7_ccm_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    IMX7CCMState *s = IMX7_CCM(obj);
+
+    memory_region_init(&s->mmio.container, obj, TYPE_IMX7_CCM,
+                       0x30000);
+
+    memory_region_init_io(&s->mmio.analog,
+                          obj,
+                          &imx7_set_clr_tog_ops,
+                          s->analog,
+                          TYPE_IMX7_CCM ".analog",
+                          sizeof(s->analog));
+
+    memory_region_add_subregion(&s->mmio.container,
+                                0x60, &s->mmio.analog);
+
+    memory_region_init_io(&s->mmio.pmu,
+                          obj,
+                          &imx7_set_clr_tog_ops,
+                          s->pmu,
+                          TYPE_IMX7_CCM ".pmu",
+                          sizeof(s->pmu));
+
+    memory_region_add_subregion(&s->mmio.container,
+                                0x200, &s->mmio.pmu);
+    
+    memory_region_init_io(&s->mmio.ccm,
+                          obj,
+                          &imx7_set_clr_tog_ops,
+                          s->ccm,
+                          TYPE_IMX7_CCM ".ccm",
+                          sizeof(s->ccm));
+
+    memory_region_add_subregion(&s->mmio.container,
+                                0x20000, &s->mmio.ccm);
+
+    sysbus_init_mmio(sd, &s->mmio.container);
+}
+
+static const VMStateDescription vmstate_imx7_ccm = {
+    .name = TYPE_IMX7_CCM,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(ccm, IMX7CCMState, CCM_MAX),
+        VMSTATE_UINT32_ARRAY(analog, IMX7CCMState, CCM_ANALOG_MAX),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void imx7_ccm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    IMXCCMClass *ccm = IMX_CCM_CLASS(klass);
+
+    dc->reset = imx7_ccm_reset;
+    dc->vmsd  = &vmstate_imx7_ccm;
+    dc->desc  = "i.MX7 Clock Control Module";
+
+    ccm->get_clock_frequency = imx7_ccm_get_clock_frequency;
+}
+
+static const TypeInfo imx7_ccm_info = {
+    .name          = TYPE_IMX7_CCM,
+    .parent        = TYPE_IMX_CCM,
+    .instance_size = sizeof(IMX7CCMState),
+    .instance_init = imx7_ccm_init,
+    .class_init    = imx7_ccm_class_init,
+};
+
+static void imx7_ccm_register_type(void)
+{
+    type_register_static(&imx7_ccm_info);
+}
+type_init(imx7_ccm_register_type)
diff --git a/include/hw/misc/imx7_ccm.h b/include/hw/misc/imx7_ccm.h
new file mode 100644
index 0000000000..72ca10a04a
--- /dev/null
+++ b/include/hw/misc/imx7_ccm.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 CCM, PMU and ANALOG IP blocks emulation code
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * 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 IMX7_CCM_H
+#define IMX7_CCM_H
+
+#include "hw/misc/imx_ccm.h"
+#include "qemu/bitops.h"
+
+#define REG_SET_CLR_TOG(name)  name, name##_SET, name##_CLR, name##_TOG
+
+enum IMX7AnalogRegisters {
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_ARM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR_SS),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR_NUM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_DDR_DENOM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_480),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_480A),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_480B),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_ENET),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO_SS),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO_NUM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_AUDIO_DENOM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO_SS),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO_NUM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_VIDEO_DENOM),
+    REG_SET_CLR_TOG(CCM_ANALOG_PLL_MISC0),
+
+    CCM_ANALOG_MAX,
+
+    CCM_ANALOG_PLL_LOCK = BIT(31)
+};
+
+enum IMX7CCMRegisters {
+    CCM_MAX = 0xBC80 / sizeof(uint32_t),
+};
+
+enum IMX7PMURegisters {
+    PMU_MAX = 0x140 / sizeof(uint32_t),
+};
+
+#undef REG_SET_CLR_TOG
+
+#define TYPE_IMX7_CCM "imx7.ccm"
+#define IMX7_CCM(obj) OBJECT_CHECK(IMX7CCMState, (obj), TYPE_IMX7_CCM)
+
+typedef struct IMX6CCMState {
+    /* <private> */
+    IMXCCMState parent_obj;
+
+    /* <public> */
+    struct {
+        MemoryRegion container;
+        MemoryRegion ccm;
+        MemoryRegion pmu;
+        MemoryRegion analog;
+    } mmio;
+
+    uint32_t ccm[CCM_MAX];
+    uint32_t pmu[PMU_MAX];
+    uint32_t analog[CCM_ANALOG_MAX];
+
+} IMX7CCMState;
+
+#endif /* IMX7_CCM_H */
-- 
2.13.5

[Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block

[Andrey Smirnov]

Add enough code to emulate i.MX2 watchdog IP block so it would be
possible to reboot the machine running Linux Guest.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/misc/Makefile.objs      |   1 +
 hw/misc/imx2_wdt.c         | 117 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/imx2_wdt.h |  36 ++++++++++++++
 3 files changed, 154 insertions(+)
 create mode 100644 hw/misc/imx2_wdt.c
 create mode 100644 include/hw/misc/imx2_wdt.h

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index ac1be05a03..c393a93456 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx25_ccm.o
 obj-$(CONFIG_IMX) += imx6_ccm.o
 obj-$(CONFIG_IMX) += imx6_src.o
 obj-$(CONFIG_IMX) += imx7_ccm.o
+obj-$(CONFIG_IMX) += imx2_wdt.o
 obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
 obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
 obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx2_wdt.c b/hw/misc/imx2_wdt.c
new file mode 100644
index 0000000000..9d97a19511
--- /dev/null
+++ b/hw/misc/imx2_wdt.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX2 Watchdog IP block
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * 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 "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "hw/hw.h"
+#include "exec/memory.h"
+#include "exec/address-spaces.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/watchdog.h"
+#include "qemu/error-report.h"
+#include "qemu/sizes.h"
+
+#include "hw/misc/imx2_wdt.h"
+
+
+#define IMX2_WDT_WCR_WDA	BIT(5)		/* -> External Reset WDOG_B */
+#define IMX2_WDT_WCR_SRS	BIT(4)		/* -> Software Reset Signal */
+
+static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
+                              unsigned int size)
+{
+    IMX2WdtState *s = opaque;
+    const size_t index = addr / sizeof(s->reg[0]);
+
+    if (index < ARRAY_SIZE(s->reg))
+        return s->reg[index];
+    else
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
+
+    return 0xDEADBEEF;
+}
+
+static void imx2_wdt_write(void *opaque, hwaddr addr,
+                           uint64_t val64, unsigned int size)
+{
+    uint16_t value  = val64;
+    IMX2WdtState *s = opaque;
+    const size_t index = addr / sizeof(s->reg[0]);
+
+    switch (index) {
+    case IMX2_WDT_WCR:
+        if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
+            watchdog_perform_action();
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
+    }
+}
+
+static const MemoryRegionOps imx2_wdt_ops = {
+    .read  = imx2_wdt_read,
+    .write = imx2_wdt_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 imx2_wdt_realize(DeviceState *dev, Error **errp)
+{
+    IMX2WdtState *s = IMX2_WDT(dev);
+
+    memory_region_init_io(&s->mmio, OBJECT(dev),
+                          &imx2_wdt_ops, s,
+                          TYPE_IMX2_WDT".mmio", SZ_64K);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
+}
+
+static void imx2_wdt_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = imx2_wdt_realize;
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo imx2_wdt_info = {
+    .name          = TYPE_IMX2_WDT,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IMX2WdtState),
+    .class_init    = imx2_wdt_class_init,
+};
+
+static WatchdogTimerModel model = {
+    .wdt_name = "imx2-watchdog",
+    .wdt_description = "i.MX2 Watchdog",
+};
+
+static void imx2_wdt_register_type(void)
+{
+    watchdog_add_model(&model);
+    type_register_static(&imx2_wdt_info);
+}
+type_init(imx2_wdt_register_type)
diff --git a/include/hw/misc/imx2_wdt.h b/include/hw/misc/imx2_wdt.h
new file mode 100644
index 0000000000..3a30ed1ef8
--- /dev/null
+++ b/include/hw/misc/imx2_wdt.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX2 Watchdog IP block
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * 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 IMX2_WDT_H
+#define IMX2_WDT_H
+
+#include "qemu/bitops.h"
+#include "hw/sysbus.h"
+
+#define TYPE_IMX2_WDT "imx2.wdt"
+#define IMX2_WDT(obj) OBJECT_CHECK(IMX2WdtState, (obj), TYPE_IMX2_WDT)
+
+enum IMX2WdtRegisters {
+    IMX2_WDT_WCR,
+    IMX2_WDT_REG_NUM
+};
+
+
+typedef struct IMX2WdtState {
+    /* <private> */
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+
+    uint16_t reg[IMX2_WDT_REG_NUM];
+} IMX2WdtState;
+
+#endif /* IMX7_SNVS_H */
-- 
2.13.5

[Qemu-devel] [PATCH 14/17] i.MX7: Add code to emulate SNVS IP-block

[Andrey Smirnov]

Add code to emulate SNVS IP-block. Currently only the bits needed to
be able to emulate machine shutdown are implemented.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/misc/Makefile.objs       |  1 +
 hw/misc/imx7_snvs.c         | 84 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/imx7_snvs.h | 35 +++++++++++++++++++
 3 files changed, 120 insertions(+)
 create mode 100644 hw/misc/imx7_snvs.c
 create mode 100644 include/hw/misc/imx7_snvs.h

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index c393a93456..16cee88e0f 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -36,6 +36,7 @@ obj-$(CONFIG_IMX) += imx6_ccm.o
 obj-$(CONFIG_IMX) += imx6_src.o
 obj-$(CONFIG_IMX) += imx7_ccm.o
 obj-$(CONFIG_IMX) += imx2_wdt.o
+obj-$(CONFIG_IMX) += imx7_snvs.o
 obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
 obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
 obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx7_snvs.c b/hw/misc/imx7_snvs.c
new file mode 100644
index 0000000000..efce0a760f
--- /dev/null
+++ b/hw/misc/imx7_snvs.c
@@ -0,0 +1,84 @@
+/*
+ * IMX7 Secure Non-Volatile Storage
+ *
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Bare minimum emulation code needed to support being able to shut
+ * down linux guest gracefully.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/sizes.h"
+#include "hw/misc/imx7_snvs.h"
+#include "qemu/log.h"
+#include "sysemu/sysemu.h"
+
+static uint64_t imx7_snvs_read(void *opaque, hwaddr offset, unsigned size)
+{
+    return 0;
+}
+
+static void imx7_snvs_write(void *opaque, hwaddr offset,
+                            uint64_t v, unsigned size)
+{
+    const uint32_t value = v;
+    const uint32_t mask  = SNVS_LPCR_TOP | SNVS_LPCR_DP_EN;
+
+    if (offset == SNVS_LPCR && ((value & mask) == mask)) {
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+    }
+}
+
+static const struct MemoryRegionOps imx7_snvs_ops = {
+    .read = imx7_snvs_read,
+    .write = imx7_snvs_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 imx7_snvs_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    IMX7SNVSState *s = IMX7_SNVS(obj);
+
+    memory_region_init_io(&s->mmio, obj, &imx7_snvs_ops, s,
+                          TYPE_IMX7_SNVS, 0x1000);
+
+    sysbus_init_mmio(sd, &s->mmio);
+}
+
+static void imx7_snvs_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc  = "i.MX7 Secure Non-Volatile Storage Module";
+}
+
+static const TypeInfo imx7_snvs_info = {
+    .name          = TYPE_IMX7_SNVS,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IMX7SNVSState),
+    .instance_init = imx7_snvs_init,
+    .class_init    = imx7_snvs_class_init,
+};
+
+static void imx7_snvs_register_type(void)
+{
+    type_register_static(&imx7_snvs_info);
+}
+type_init(imx7_snvs_register_type)
diff --git a/include/hw/misc/imx7_snvs.h b/include/hw/misc/imx7_snvs.h
new file mode 100644
index 0000000000..255f8f26f9
--- /dev/null
+++ b/include/hw/misc/imx7_snvs.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 SNVS block emulation code
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * 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 IMX7_SNVS_H
+#define IMX7_SNVS_H
+
+#include "qemu/bitops.h"
+#include "hw/sysbus.h"
+
+
+enum IMX7SNVSRegisters {
+    SNVS_LPCR = 0x38,
+    SNVS_LPCR_TOP   = BIT(6),
+    SNVS_LPCR_DP_EN = BIT(5)
+};
+
+#define TYPE_IMX7_SNVS "imx7.snvs"
+#define IMX7_SNVS(obj) OBJECT_CHECK(IMX7SNVSState, (obj), TYPE_IMX7_SNVS)
+
+typedef struct IMX7SNVSState {
+    /* <private> */
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+} IMX7SNVSState;
+
+#endif /* IMX7_SNVS_H */
-- 
2.13.5

[Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux

[Andrey Smirnov]

Add sizes.h from Linux to have a more readable way of specifying
MemoryRegion sizes.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/qemu/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 include/qemu/sizes.h

diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
new file mode 100644
index 0000000000..9aedb9f8f6
--- /dev/null
+++ b/include/qemu/sizes.h
@@ -0,0 +1,47 @@
+/*
+ * Copy of include/linux/sizes.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef SIZES_H
+#define SIZES_H
+
+#define SZ_1				0x00000001
+#define SZ_2				0x00000002
+#define SZ_4				0x00000004
+#define SZ_8				0x00000008
+#define SZ_16				0x00000010
+#define SZ_32				0x00000020
+#define SZ_64				0x00000040
+#define SZ_128				0x00000080
+#define SZ_256				0x00000100
+#define SZ_512				0x00000200
+
+#define SZ_1K				0x00000400
+#define SZ_2K				0x00000800
+#define SZ_4K				0x00001000
+#define SZ_8K				0x00002000
+#define SZ_16K				0x00004000
+#define SZ_32K				0x00008000
+#define SZ_64K				0x00010000
+#define SZ_128K				0x00020000
+#define SZ_256K				0x00040000
+#define SZ_512K				0x00080000
+
+#define SZ_1M				0x00100000
+#define SZ_2M				0x00200000
+#define SZ_4M				0x00400000
+#define SZ_8M				0x00800000
+#define SZ_16M				0x01000000
+#define SZ_32M				0x02000000
+#define SZ_64M				0x04000000
+#define SZ_128M				0x08000000
+#define SZ_256M				0x10000000
+#define SZ_512M				0x20000000
+
+#define SZ_1G				0x40000000
+#define SZ_2G				0x80000000
+
+#endif /* SIZES_H */
-- 
2.13.5

[Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.

[Andrey Smirnov]

For now we only support the following devices:
    * up to 2 Cortex A9 cores (SMP works with PSCI)
    * A7 MPCORE (identical to A15 MPCORE)
    * 7 i.MX UARTs
    * 1 CCM device
    * 2 Ethernet controllers (FEC)
    * 3 SD controllers (USDHC)
    * 1 SNVS device
    * 1 WDT device

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 default-configs/arm-softmmu.mak |   1 +
 hw/arm/Makefile.objs            |   2 +
 hw/arm/fsl-imx7.c               | 327 ++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/fsl-imx7.h       | 114 ++++++++++++++
 4 files changed, 444 insertions(+)
 create mode 100644 hw/arm/fsl-imx7.c
 create mode 100644 include/hw/arm/fsl-imx7.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index bbdd3c1d8b..98396a3ad2 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
 CONFIG_FSL_IMX6=y
 CONFIG_FSL_IMX31=y
 CONFIG_FSL_IMX25=y
+CONFIG_FSL_IMX7=y
 
 CONFIG_IMX_I2C=y
 
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index a2e56ecaae..33f6051ae3 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
 obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
 obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
 obj-$(CONFIG_MPS2) += mps2.o
+obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
+
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
new file mode 100644
index 0000000000..bd01bb7f59
--- /dev/null
+++ b/hw/arm/fsl-imx7.c
@@ -0,0 +1,327 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 SoC definitions
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * Based on hw/arm/fsl-imx6.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "hw/arm/fsl-imx7.h"
+#include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
+
+#define NAME_SIZE 20
+
+#define for_each_cpu(i)     for (i = 0; i < smp_cpus; i++)
+
+static void fsl_imx7_init(Object *obj)
+{
+    BusState *sysbus = sysbus_get_default();
+    FslIMX7State *s = FSL_IMX7(obj);
+    char name[NAME_SIZE];
+    int i;
+
+    if (smp_cpus > FSL_IMX7_NUM_CPUS) {
+        error_report("%s: Only %d CPUs are supported (%d requested)",
+                     TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
+        exit(1);
+    }
+
+    for_each_cpu(i) {
+        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
+                          "cortex-a7-" TYPE_ARM_CPU);
+        snprintf(name, NAME_SIZE, "cpu%d", i);
+        object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
+                                  &error_fatal);
+    }
+
+    /*
+     * A7MPCORE
+     */
+    object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);
+    qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
+    object_property_add_child(obj, "a7mpcore",
+                              OBJECT(&s->a7mpcore), &error_fatal);
+
+    /*
+     * CCM
+     */
+    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
+    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
+    object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
+
+    /*
+     * UART
+     */
+    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
+            object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
+            qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
+            snprintf(name, NAME_SIZE, "uart%d", i);
+            object_property_add_child(obj, name, OBJECT(&s->uart[i]),
+                                      &error_fatal);
+    }
+
+    /*
+     * Ethernet
+     */
+    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
+            object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
+            qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
+            snprintf(name, NAME_SIZE, "eth%d", i);
+            object_property_add_child(obj, name, OBJECT(&s->eth[i]),
+                                      &error_fatal);
+    }
+
+    /*
+     * SDHCI
+     */
+    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
+            object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
+                              TYPE_IMX_USDHC);
+            qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
+            snprintf(name, NAME_SIZE, "usdhc%d", i);
+            object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
+                                      &error_fatal);
+    }
+
+    /*
+     * SNVS
+     */
+    object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
+    qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
+    object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
+
+    /*
+     * Watchdog
+     */
+    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
+            object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
+            qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
+            snprintf(name, NAME_SIZE, "wdt%d", i);
+            object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
+                                      &error_fatal);
+    }
+}
+
+static void fsl_imx7_realize(DeviceState *dev, Error **errp)
+{
+    FslIMX7State *s = FSL_IMX7(dev);
+    Object *o;
+    int i;
+    qemu_irq irq;
+
+    for_each_cpu(i) {
+	o = OBJECT(&s->cpu[i]);
+
+        object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
+                                "psci-conduit", &error_abort);
+
+        object_property_set_bool(o, false, "has_el3", &error_abort);
+
+        /* On uniprocessor, the CBAR is set to 0 */
+        if (smp_cpus > 1) {
+            object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
+                                    "reset-cbar", &error_abort);
+        }
+
+        if (i) {
+            /* Secondary CPUs start in PSCI powered-down state */
+            object_property_set_bool(o, true,
+                                     "start-powered-off", &error_abort);
+        }
+
+        object_property_set_bool(o, true, "realized", &error_abort);
+    }
+
+    /*
+     * A7MPCORE
+     */
+    object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
+                            &error_abort);
+    object_property_set_int(OBJECT(&s->a7mpcore),
+                            FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
+                            "num-irq", &error_abort);
+
+    object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
+                             &error_abort);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
+
+    for_each_cpu(i) {
+        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
+        DeviceState  *d   = DEVICE(qemu_get_cpu(i));
+
+        irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
+        sysbus_connect_irq(sbd, i, irq);
+        irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
+        sysbus_connect_irq(sbd, i + smp_cpus, irq);
+    }
+
+    /*
+     * CCM
+     */
+    object_property_set_bool(OBJECT(&s->ccm), true, "realized", &error_abort);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
+
+    /*
+     * UART
+     */
+    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
+        static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
+            FSL_IMX7_UART1_ADDR,
+            FSL_IMX7_UART2_ADDR,
+            FSL_IMX7_UART3_ADDR,
+            FSL_IMX7_UART4_ADDR,
+            FSL_IMX7_UART5_ADDR,
+            FSL_IMX7_UART6_ADDR,
+            FSL_IMX7_UART7_ADDR,
+        };
+
+        static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
+            FSL_IMX7_UART1_IRQ,
+            FSL_IMX7_UART2_IRQ,
+            FSL_IMX7_UART3_IRQ,
+            FSL_IMX7_UART4_IRQ,
+            FSL_IMX7_UART5_IRQ,
+            FSL_IMX7_UART6_IRQ,
+            FSL_IMX7_UART7_IRQ,
+        };
+
+
+        if (i < MAX_SERIAL_PORTS) {
+            Chardev *chr;
+            chr = serial_hds[i];
+
+            if (!chr) {
+                char *label = g_strdup_printf("imx7.uart%d", i + 1);
+                chr = qemu_chr_new(label, "null");
+                g_free(label);
+                serial_hds[i] = chr;
+            }
+
+            qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
+        }
+
+        object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
+                                 &error_abort);
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, FSL_IMX7_UARTn_ADDR[i]);
+
+        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
+    }
+
+    /*
+     * Etherenet
+     */
+    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
+        static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
+            FSL_IMX7_ENET1_ADDR,
+            FSL_IMX7_ENET2_ADDR,
+        };
+
+        qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
+        object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
+                                 &error_abort);
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
+
+        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 0));
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
+        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 3));
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
+    }
+
+    /*
+     * USDHC
+     */
+    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
+        static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
+            FSL_IMX7_USDHC1_ADDR,
+            FSL_IMX7_USDHC2_ADDR,
+            FSL_IMX7_USDHC3_ADDR,
+        };
+
+        static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
+            FSL_IMX7_USDHC1_IRQ,
+            FSL_IMX7_USDHC2_IRQ,
+            FSL_IMX7_USDHC3_IRQ,
+        };
+
+        object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
+                                 &error_abort);
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
+                        FSL_IMX7_USDHCn_ADDR[i]);
+
+        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
+    }
+
+    /*
+     * SNVS
+     */
+    object_property_set_bool(OBJECT(&s->snvs), true, "realized", &error_abort);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
+
+    /*
+     * Watchdog
+     */
+    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
+        static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
+            FSL_IMX7_WDOG1_ADDR,
+            FSL_IMX7_WDOG2_ADDR,
+            FSL_IMX7_WDOG3_ADDR,
+            FSL_IMX7_WDOG4_ADDR,
+        };
+
+        object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
+                                 &error_abort);
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, FSL_IMX7_WDOGn_ADDR[i]);
+    }
+}
+
+static void fsl_imx7_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = fsl_imx7_realize;
+
+    /*
+     * Reason: creates an ARM CPU, thus use after free(), see
+     * arm_cpu_class_init()
+     */
+    dc->user_creatable = false;
+    dc->desc = "i.MX7 SOC";
+}
+
+static const TypeInfo fsl_imx7_type_info = {
+    .name = TYPE_FSL_IMX7,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(FslIMX7State),
+    .instance_init = fsl_imx7_init,
+    .class_init = fsl_imx7_class_init,
+};
+
+static void fsl_imx7_register_types(void)
+{
+    type_register_static(&fsl_imx7_type_info);
+}
+type_init(fsl_imx7_register_types)
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
new file mode 100644
index 0000000000..0f3d43e379
--- /dev/null
+++ b/include/hw/arm/fsl-imx7.h
@@ -0,0 +1,114 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 SoC definitions
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef FSL_IMX7_H
+#define FSL_IMX7_H
+
+#include "hw/arm/arm.h"
+#include "hw/cpu/a15mpcore.h"
+#include "hw/misc/imx7_ccm.h"
+#include "hw/misc/imx7_snvs.h"
+#include "hw/misc/imx6_src.h"
+#include "hw/misc/imx2_wdt.h"
+#include "hw/char/imx_serial.h"
+#include "hw/timer/imx_gpt.h"
+#include "hw/timer/imx_epit.h"
+#include "hw/i2c/imx_i2c.h"
+#include "hw/gpio/imx_gpio.h"
+#include "hw/sd/sdhci.h"
+#include "hw/ssi/imx_spi.h"
+#include "hw/net/imx_fec.h"
+#include "exec/memory.h"
+#include "cpu.h"
+#include "qemu/sizes.h"
+
+#define TYPE_FSL_IMX7 "fsl,imx7"
+#define FSL_IMX7(obj) OBJECT_CHECK(FslIMX7State, (obj), TYPE_FSL_IMX7)
+
+enum FslIMX7Configuration {
+    FSL_IMX7_NUM_CPUS  = 2,
+    FSL_IMX7_NUM_UARTS = 7,
+    FSL_IMX7_NUM_ETHS  = 2,
+    FSL_IMX7_NUM_USDHCS = 3,
+    FSL_IMX7_NUM_WDTS = 4,
+};
+
+typedef struct FslIMX7State {
+    /*< private >*/
+    DeviceState    parent_obj;
+
+    /*< public >*/
+    ARMCPU         cpu[FSL_IMX7_NUM_CPUS];
+    A15MPPrivState a7mpcore;
+    IMX7CCMState   ccm;
+    IMX7SNVSState  snvs;
+    IMXSerialState uart[FSL_IMX7_NUM_UARTS];
+    IMXFECState    eth[FSL_IMX7_NUM_ETHS];
+    SDHCIState     usdhc[FSL_IMX7_NUM_USDHCS];
+    IMX2WdtState   wdt[FSL_IMX7_NUM_WDTS];
+} FslIMX7State;
+
+enum FslIMX7MemoryMap {
+    FSL_IMX7_MMDC_ADDR      = 0x80000000,
+    FSL_IMX7_MMDC_SIZE      = SZ_2G,
+
+    FSL_IMX7_WDOG1_ADDR     = 0x30280000,
+    FSL_IMX7_WDOG2_ADDR     = 0x30290000,
+    FSL_IMX7_WDOG3_ADDR     = 0x302A0000,
+    FSL_IMX7_WDOG4_ADDR     = 0x302B0000,
+
+    FSL_IMX7_CCM_ADDR       = 0x30360000,
+    FSL_IMX7_SNVS_ADDR	    = 0x30370000,
+
+    FSL_IMX7_UART1_ADDR     = 0x30860000,
+    FSL_IMX7_UART2_ADDR     = 0x30870000,
+    FSL_IMX7_UART3_ADDR     = 0x30880000,
+    FSL_IMX7_UART4_ADDR     = 0x30A60000,
+    FSL_IMX7_UART5_ADDR     = 0x30A70000,
+    FSL_IMX7_UART6_ADDR     = 0x30A80000,
+    FSL_IMX7_UART7_ADDR     = 0x30A90000,
+
+    FSL_IMX7_ENET1_ADDR     = 0x30BE0000,
+    FSL_IMX7_ENET2_ADDR     = 0x30BF0000,
+
+    FSL_IMX7_USDHC1_ADDR    = 0x30B40000,
+    FSL_IMX7_USDHC2_ADDR    = 0x30B50000,
+    FSL_IMX7_USDHC3_ADDR    = 0x30B60000,
+
+    FSL_IMX7_A7MPCORE_ADDR  = 0x31000000,
+};
+
+enum FslIMX7IRQs {
+
+    FSL_IMX7_USDHC1_IRQ = 22,
+    FSL_IMX7_USDHC2_IRQ = 23,
+    FSL_IMX7_USDHC3_IRQ = 24,
+
+    FSL_IMX7_UART1_IRQ = 26,
+    FSL_IMX7_UART2_IRQ = 27,
+    FSL_IMX7_UART3_IRQ = 28,
+    FSL_IMX7_UART4_IRQ = 29,
+    FSL_IMX7_UART5_IRQ = 30,
+    FSL_IMX7_UART6_IRQ = 16,
+    FSL_IMX7_UART7_IRQ = 126,
+
+#define FSL_IMX7_ENET_IRQ(i, n)  ((n) + ((i) ? 100 : 118))
+
+    FSL_IMX7_MAX_IRQ   = 128,
+};
+
+#endif /* FSL_IMX7_H */
-- 
2.13.5

[Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board

[Andrey Smirnov]

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org
Cc: qemu-arm@nongnu.org
Cc: yurovsky@gmail.com
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 hw/arm/Makefile.objs   |   2 +-
 hw/arm/mcimx7d-sabre.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/mcimx7d-sabre.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 33f6051ae3..fc4a963de8 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
 obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
 obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
 obj-$(CONFIG_MPS2) += mps2.o
-obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
+obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
 
diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
new file mode 100644
index 0000000000..34e3933db8
--- /dev/null
+++ b/hw/arm/mcimx7d-sabre.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * MCIMX7D_SABRE Board System emulation.
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This code is licensed under the GPL, version 2 or later.
+ * See the file `COPYING' in the top level directory.
+ *
+ * It (partially) emulates a mcimx7d_sabre board, with a Freescale
+ * i.MX7 SoC
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "hw/arm/fsl-imx7.h"
+#include "hw/boards.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/device_tree.h"
+#include "qemu/error-report.h"
+#include "sysemu/qtest.h"
+#include "net/net.h"
+
+typedef struct {
+    FslIMX7State soc;
+    MemoryRegion ram;
+} MCIMX7Sabre;
+
+static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
+{
+    const char comp[] = "arm,psci-0.2\0arm,psci";
+
+    qemu_fdt_add_subnode(fdt, "/psci");
+    qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
+    qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
+}
+
+static void mcimx7d_sabre_init(MachineState *machine)
+{
+    static struct arm_boot_info boot_info;
+    MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
+    Object *soc;
+    int i;
+
+    if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
+        error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
+                     machine->ram_size, FSL_IMX7_MMDC_SIZE);
+        exit(1);
+    }
+
+    boot_info = (struct arm_boot_info) {
+        .loader_start = FSL_IMX7_MMDC_ADDR,
+        .board_id = -1,
+        .ram_size = machine->ram_size,
+        .kernel_filename = machine->kernel_filename,
+        .kernel_cmdline = machine->kernel_cmdline,
+        .initrd_filename = machine->initrd_filename,
+        .nb_cpus = smp_cpus,
+        .modify_dtb = mcimx7d_add_psci_node,
+    };
+
+    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
+    soc = OBJECT(&s->soc);
+    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
+    object_property_set_bool(soc, true, "realized", &error_fatal);
+
+    memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
+                                         machine->ram_size);
+    memory_region_add_subregion(get_system_memory(),
+                                FSL_IMX7_MMDC_ADDR, &s->ram);
+
+    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
+        BusState *bus;
+        DeviceState *carddev;
+        DriveInfo *di;
+        BlockBackend *blk;
+
+        di = drive_get_next(IF_SD);
+        blk = di ? blk_by_legacy_dinfo(di) : NULL;
+        bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
+        carddev = qdev_create(bus, TYPE_SD_CARD);
+        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+        object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
+    }
+
+    if (!qtest_enabled()) {
+        arm_load_kernel(&s->soc.cpu[0], &boot_info);
+    }
+}
+
+static void mcimx7d_sabre_machine_init(MachineClass *mc)
+{
+    mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
+    mc->init = mcimx7d_sabre_init;
+    mc->max_cpus = FSL_IMX7_NUM_CPUS;
+    mc->ignore_memory_transaction_failures = true;
+}
+DEFINE_MACHINE("mcimx7d-sabre", mcimx7d_sabre_machine_init)
-- 
2.13.5

Re: [Qemu-devel] [PATCH 00/17] Initial i.MX7 support

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 00/17] Initial i.MX7 support
Message-id: 20170918195100.17593-1-andrew.smirnov@gmail.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20170918202529.28379-1-el13635@mail.ntua.gr -> patchew/20170918202529.28379-1-el13635@mail.ntua.gr
Switched to a new branch 'test'
274d7165b1 Implement support for i.MX7 Sabre board
d24ef9c251 i.MX: Add i.MX7 SOC implementation.
74a0b2d896 include/qemu: Add sizes.h from Linux
920e37dac2 i.MX7: Add code to emulate SNVS IP-block
0d876ce354 i.MX: Add code to emulate i.MX2 watchdog IP block
e21f310ca0 i.MX: Add i.MX7 CCM, PMU and ANALOG device
e5d8486d8c sdhci: Implement write method of ACMD12ERRSTS register
c00331bbb7 sdhci: Add i.MX specific subtype of SDHCI
9b01e35230 imx_fec: Use correct length for packet size
47351ad73c imx_fec: Add support for multiple Tx DMA rings
f23e9656f5 imx_fec: Emulate SHIFT16 in ENETx_RACC
abc29e9048 imx_fec: Use MIN instead of explicit ternary operator
456a2681ea imx_fec: Use ENET_FTRL to determine truncation length
f756d1961b imx_fec: Change queue flushing heuristics
e3f400de53 imx_fec: Refactor imx_eth_enable_rx()
38f4f4a46c imx_fec: Do not calculate FEC
e07552c97b imx_fec: Do not link to netdev

=== OUTPUT BEGIN ===
Checking PATCH 1/17: imx_fec: Do not link to netdev...
Checking PATCH 2/17: imx_fec: Do not calculate FEC...
Checking PATCH 3/17: imx_fec: Refactor imx_eth_enable_rx()...
Checking PATCH 4/17: imx_fec: Change queue flushing heuristics...
Checking PATCH 5/17: imx_fec: Use ENET_FTRL to determine truncation length...
Checking PATCH 6/17: imx_fec: Use MIN instead of explicit ternary operator...
Checking PATCH 7/17: imx_fec: Emulate SHIFT16 in ENETx_RACC...
ERROR: code indent should never use tabs
#73: FILE: include/hw/net/imx_fec.h:172:
+#define ENET_RACC_SHIFT16^IBIT(7)$

total: 1 errors, 0 warnings, 49 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 8/17: imx_fec: Add support for multiple Tx DMA rings...
WARNING: line over 80 characters
#249: FILE: include/hw/net/imx_fec.h:123:
+                                ENET_INT_TS_AVAIL | ENET_INT_TXF1 | ENET_INT_TXB1 |\

total: 0 errors, 1 warnings, 241 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 9/17: imx_fec: Use correct length for packet size...
Checking PATCH 10/17: sdhci: Add i.MX specific subtype of SDHCI...
ERROR: code indent should never use tabs
#37: FILE: hw/sd/sdhci-internal.h:235:
+#define ESDHC_MIX_CTRL^I^I^I0x48$

ERROR: code indent should never use tabs
#38: FILE: hw/sd/sdhci-internal.h:236:
+#define ESDHC_VENDOR_SPEC^I^I0xc0$

ERROR: code indent should never use tabs
#39: FILE: hw/sd/sdhci-internal.h:237:
+#define ESDHC_DLL_CTRL^I^I^I0x60$

ERROR: code indent should never use tabs
#41: FILE: hw/sd/sdhci-internal.h:239:
+#define ESDHC_TUNING_CTRL^I^I0xcc$

ERROR: code indent should never use tabs
#42: FILE: hw/sd/sdhci-internal.h:240:
+#define ESDHC_TUNE_CTRL_STATUS^I^I0x68$

ERROR: code indent should never use tabs
#43: FILE: hw/sd/sdhci-internal.h:241:
+#define ESDHC_WTMK_LVL^I^I^I0x44$

ERROR: code indent should never use tabs
#45: FILE: hw/sd/sdhci-internal.h:243:
+#define ESDHC_CTRL_4BITBUS^I^I(0x1 << 1)$

ERROR: code indent should never use tabs
#46: FILE: hw/sd/sdhci-internal.h:244:
+#define ESDHC_CTRL_8BITBUS^I^I(0x2 << 1)$

ERROR: braces {} are necessary for all arms of this statement
#99: FILE: hw/sd/sdhci.c:1405:
+        if (s->hostctl & SDHC_CTRL_8BITBUS)
[...]

ERROR: braces {} are necessary for all arms of this statement
#102: FILE: hw/sd/sdhci.c:1408:
+        if (s->hostctl & SDHC_CTRL_4BITBUS)
[...]

ERROR: braces {} are necessary for all arms of this statement
#141: FILE: hw/sd/sdhci.c:1447:
+        if (value & ESDHC_CTRL_8BITBUS)
[...]

ERROR: braces {} are necessary for all arms of this statement
#144: FILE: hw/sd/sdhci.c:1450:
+        if (value & ESDHC_CTRL_4BITBUS)
[...]

ERROR: code indent should never use tabs
#231: FILE: include/hw/sd/sdhci.h:92:
+#define SDHCI_QUIRK_NO_BUSY_IRQ^I^I^I^I(1<<14)$

ERROR: spaces required around that '<<' (ctx:VxV)
#231: FILE: include/hw/sd/sdhci.h:92:
+#define SDHCI_QUIRK_NO_BUSY_IRQ				(1<<14)
                                				  ^

total: 14 errors, 0 warnings, 203 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 11/17: sdhci: Implement write method of ACMD12ERRSTS register...
Checking PATCH 12/17: i.MX: Add i.MX7 CCM, PMU and ANALOG device...
ERROR: trailing whitespace
#71: FILE: hw/misc/imx7_ccm.c:41:
+    s->analog[CCM_ANALOG_PLL_480]         = 0x00002000;    $

ERROR: code indent should never use tabs
#97: FILE: hw/misc/imx7_ccm.c:67:
+#define CCM_INDEX(offset)^I(((offset) & ~(hwaddr)0xF) / sizeof(uint32_t))$

ERROR: code indent should never use tabs
#98: FILE: hw/misc/imx7_ccm.c:68:
+#define CCM_BITOP(offset)^I((offset) & (hwaddr)0xF)$

ERROR: trailing whitespace
#182: FILE: hw/misc/imx7_ccm.c:152:
+    $

ERROR: Macros with complex values should be enclosed in parenthesis
#255: FILE: include/hw/misc/imx7_ccm.h:18:
+#define REG_SET_CLR_TOG(name)  name, name##_SET, name##_CLR, name##_TOG

total: 5 errors, 0 warnings, 284 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 13/17: i.MX: Add code to emulate i.MX2 watchdog IP block...
ERROR: code indent should never use tabs
#62: FILE: hw/misc/imx2_wdt.c:29:
+#define IMX2_WDT_WCR_WDA^IBIT(5)^I^I/* -> External Reset WDOG_B */$

ERROR: code indent should never use tabs
#63: FILE: hw/misc/imx2_wdt.c:30:
+#define IMX2_WDT_WCR_SRS^IBIT(4)^I^I/* -> Software Reset Signal */$

ERROR: braces {} are necessary for all arms of this statement
#71: FILE: hw/misc/imx2_wdt.c:38:
+    if (index < ARRAY_SIZE(s->reg))
[...]
+    else
[...]

ERROR: braces {} are necessary for all arms of this statement
#89: FILE: hw/misc/imx2_wdt.c:56:
+        if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
[...]

total: 4 errors, 0 warnings, 160 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 14/17: i.MX7: Add code to emulate SNVS IP-block...
Checking PATCH 15/17: include/qemu: Add sizes.h from Linux...
ERROR: code indent should never use tabs
#32: FILE: include/qemu/sizes.h:11:
+#define SZ_1^I^I^I^I0x00000001$

ERROR: code indent should never use tabs
#33: FILE: include/qemu/sizes.h:12:
+#define SZ_2^I^I^I^I0x00000002$

ERROR: code indent should never use tabs
#34: FILE: include/qemu/sizes.h:13:
+#define SZ_4^I^I^I^I0x00000004$

ERROR: code indent should never use tabs
#35: FILE: include/qemu/sizes.h:14:
+#define SZ_8^I^I^I^I0x00000008$

ERROR: code indent should never use tabs
#36: FILE: include/qemu/sizes.h:15:
+#define SZ_16^I^I^I^I0x00000010$

ERROR: code indent should never use tabs
#37: FILE: include/qemu/sizes.h:16:
+#define SZ_32^I^I^I^I0x00000020$

ERROR: code indent should never use tabs
#38: FILE: include/qemu/sizes.h:17:
+#define SZ_64^I^I^I^I0x00000040$

ERROR: code indent should never use tabs
#39: FILE: include/qemu/sizes.h:18:
+#define SZ_128^I^I^I^I0x00000080$

ERROR: code indent should never use tabs
#40: FILE: include/qemu/sizes.h:19:
+#define SZ_256^I^I^I^I0x00000100$

ERROR: code indent should never use tabs
#41: FILE: include/qemu/sizes.h:20:
+#define SZ_512^I^I^I^I0x00000200$

ERROR: code indent should never use tabs
#43: FILE: include/qemu/sizes.h:22:
+#define SZ_1K^I^I^I^I0x00000400$

ERROR: code indent should never use tabs
#44: FILE: include/qemu/sizes.h:23:
+#define SZ_2K^I^I^I^I0x00000800$

ERROR: code indent should never use tabs
#45: FILE: include/qemu/sizes.h:24:
+#define SZ_4K^I^I^I^I0x00001000$

ERROR: code indent should never use tabs
#46: FILE: include/qemu/sizes.h:25:
+#define SZ_8K^I^I^I^I0x00002000$

ERROR: code indent should never use tabs
#47: FILE: include/qemu/sizes.h:26:
+#define SZ_16K^I^I^I^I0x00004000$

ERROR: code indent should never use tabs
#48: FILE: include/qemu/sizes.h:27:
+#define SZ_32K^I^I^I^I0x00008000$

ERROR: code indent should never use tabs
#49: FILE: include/qemu/sizes.h:28:
+#define SZ_64K^I^I^I^I0x00010000$

ERROR: code indent should never use tabs
#50: FILE: include/qemu/sizes.h:29:
+#define SZ_128K^I^I^I^I0x00020000$

ERROR: code indent should never use tabs
#51: FILE: include/qemu/sizes.h:30:
+#define SZ_256K^I^I^I^I0x00040000$

ERROR: code indent should never use tabs
#52: FILE: include/qemu/sizes.h:31:
+#define SZ_512K^I^I^I^I0x00080000$

ERROR: code indent should never use tabs
#54: FILE: include/qemu/sizes.h:33:
+#define SZ_1M^I^I^I^I0x00100000$

ERROR: code indent should never use tabs
#55: FILE: include/qemu/sizes.h:34:
+#define SZ_2M^I^I^I^I0x00200000$

ERROR: code indent should never use tabs
#56: FILE: include/qemu/sizes.h:35:
+#define SZ_4M^I^I^I^I0x00400000$

ERROR: code indent should never use tabs
#57: FILE: include/qemu/sizes.h:36:
+#define SZ_8M^I^I^I^I0x00800000$

ERROR: code indent should never use tabs
#58: FILE: include/qemu/sizes.h:37:
+#define SZ_16M^I^I^I^I0x01000000$

ERROR: code indent should never use tabs
#59: FILE: include/qemu/sizes.h:38:
+#define SZ_32M^I^I^I^I0x02000000$

ERROR: code indent should never use tabs
#60: FILE: include/qemu/sizes.h:39:
+#define SZ_64M^I^I^I^I0x04000000$

ERROR: code indent should never use tabs
#61: FILE: include/qemu/sizes.h:40:
+#define SZ_128M^I^I^I^I0x08000000$

ERROR: code indent should never use tabs
#62: FILE: include/qemu/sizes.h:41:
+#define SZ_256M^I^I^I^I0x10000000$

ERROR: code indent should never use tabs
#63: FILE: include/qemu/sizes.h:42:
+#define SZ_512M^I^I^I^I0x20000000$

ERROR: code indent should never use tabs
#65: FILE: include/qemu/sizes.h:44:
+#define SZ_1G^I^I^I^I0x40000000$

ERROR: code indent should never use tabs
#66: FILE: include/qemu/sizes.h:45:
+#define SZ_2G^I^I^I^I0x80000000$

total: 32 errors, 0 warnings, 47 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 16/17: i.MX: Add i.MX7 SOC implementation....
ERROR: code indent should never use tabs
#178: FILE: hw/arm/fsl-imx7.c:128:
+^Io = OBJECT(&s->cpu[i]);$

ERROR: code indent should never use tabs
#458: FILE: include/hw/arm/fsl-imx7.h:75:
+    FSL_IMX7_SNVS_ADDR^I    = 0x30370000,$

total: 2 errors, 0 warnings, 453 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 17/17: Implement support for i.MX7 Sabre board...
WARNING: line over 80 characters
#60: FILE: hw/arm/mcimx7d-sabre.c:31:
+static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)

WARNING: line over 80 characters
#114: FILE: hw/arm/mcimx7d-sabre.c:85:
+        object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);

total: 0 errors, 2 warnings, 106 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics

[Jason Wang]

On 2017年09月19日 03:50, Andrey Smirnov wrote:
> In current implementation, packet queue flushing logic seem to suffer
> from a deadlock like scenario if a packet is received by the interface
> before before Rx ring is initialized by Guest's driver. Consider the
> following sequence of events:
>
> 	1. A QEMU instance is started against a TAP device on Linux
> 	   host, running Linux guest, e. g., something to the effect
> 	   of:
>
> 	   qemu-system-arm \
> 	      -net nic,model=imx.fec,netdev=lan0 \
> 	      netdev tap,id=lan0,ifname=tap0,script=no,downscript=no \
> 	      ... rest of the arguments ...
>
> 	2. Once QEMU starts, but before guest reaches the point where
> 	   FEC deriver is done initializing the HW, Guest, via TAP
> 	   interface, receives a number of multicast MDNS packets from
> 	   Host (not necessarily true for every OS, but it happens at
> 	   least on Fedora 25)
>
> 	3. Recieving a packet in such a state results in
> 	   imx_eth_can_receive() returning '0', which in turn causes
> 	   tap_send() to disable corresponding event (tap.c:203)
>
> 	4. Once Guest's driver reaches the point where it is ready to
> 	   recieve packets it prepares Rx ring descriptors and writes
> 	   ENET_RDAR_RDAR to ENET_RDAR register to indicate to HW that
> 	   more descriptors are ready. And at this points emulation
> 	   layer does this:
>
> 	   	 s->regs[index] = ENET_RDAR_RDAR;
>                   imx_eth_enable_rx(s);
>
> 	   which, combined with:
>
> 	   	  if (!s->regs[ENET_RDAR]) {
> 		     qemu_flush_queued_packets(qemu_get_queue(s->nic));
> 		  }

Not familiar with FEC, but if you are tracking 0->1 transition, why not 
simply introduce a parameter of imx_eth_enable_rx() to force the flushing?

Thanks

>
> 	   results in Rx queue never being flushed and corresponding
> 	   I/O event beign disabled.
>
> Change the code to remember the fact that can_receive callback was
> called before Rx ring was ready and use it to make a decision if
> receive queue needs to be flushed.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>   hw/net/imx_fec.c         | 6 ++++--
>   include/hw/net/imx_fec.h | 1 +
>   2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 84085afe09..767402909d 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -544,8 +544,9 @@ static void imx_eth_enable_rx(IMXFECState *s)
>   
>       if (rx_ring_full) {
>           FEC_PRINTF("RX buffer full\n");
> -    } else if (!s->regs[ENET_RDAR]) {
> +    } else if (s->needs_flush) {
>           qemu_flush_queued_packets(qemu_get_queue(s->nic));
> +        s->needs_flush = false;
>       }
>   
>       s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
> @@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
>   
>       FEC_PRINTF("\n");
>   
> -    return s->regs[ENET_RDAR] ? 1 : 0;
> +    s->needs_flush = !s->regs[ENET_RDAR];
> +    return !!s->regs[ENET_RDAR];
>   }
>   
>   static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 62ad473b05..4bc8f03ec2 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -252,6 +252,7 @@ typedef struct IMXFECState {
>       uint32_t phy_int_mask;
>   
>       bool is_fec;
> +    bool needs_flush;
>   } IMXFECState;
>   
>   #endif

Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings

[Jason Wang]

On 2017年09月19日 03:50, Andrey Smirnov wrote:
> More recent version of the IP block support more than one Tx DMA ring,
> so add the code implementing that feature.
>
> Cc: Peter Maydell<peter.maydell@linaro.org>
> Cc: Jason Wang<jasowang@redhat.com>
> Cc:qemu-devel@nongnu.org
> Cc:qemu-arm@nongnu.org
> Cc:yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov<andrew.smirnov@gmail.com>
> ---
>   hw/net/imx_fec.c         | 97 +++++++++++++++++++++++++++++++++++++++---------
>   include/hw/net/imx_fec.h | 23 +++++++++++-
>   2 files changed, 101 insertions(+), 19 deletions(-)

Is there a register for driver to know about the version? (Looks like I 
didn't find it).

Thanks

Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics

[Andrey Smirnov]

On Fri, Sep 22, 2017 at 12:27 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年09月19日 03:50, Andrey Smirnov wrote:
>>
>> In current implementation, packet queue flushing logic seem to suffer
>> from a deadlock like scenario if a packet is received by the interface
>> before before Rx ring is initialized by Guest's driver. Consider the
>> following sequence of events:
>>
>>         1. A QEMU instance is started against a TAP device on Linux
>>            host, running Linux guest, e. g., something to the effect
>>            of:
>>
>>            qemu-system-arm \
>>               -net nic,model=imx.fec,netdev=lan0 \
>>               netdev tap,id=lan0,ifname=tap0,script=no,downscript=no \
>>               ... rest of the arguments ...
>>
>>         2. Once QEMU starts, but before guest reaches the point where
>>            FEC deriver is done initializing the HW, Guest, via TAP
>>            interface, receives a number of multicast MDNS packets from
>>            Host (not necessarily true for every OS, but it happens at
>>            least on Fedora 25)
>>
>>         3. Recieving a packet in such a state results in
>>            imx_eth_can_receive() returning '0', which in turn causes
>>            tap_send() to disable corresponding event (tap.c:203)
>>
>>         4. Once Guest's driver reaches the point where it is ready to
>>            recieve packets it prepares Rx ring descriptors and writes
>>            ENET_RDAR_RDAR to ENET_RDAR register to indicate to HW that
>>            more descriptors are ready. And at this points emulation
>>            layer does this:
>>
>>                  s->regs[index] = ENET_RDAR_RDAR;
>>                   imx_eth_enable_rx(s);
>>
>>            which, combined with:
>>
>>                   if (!s->regs[ENET_RDAR]) {
>>                      qemu_flush_queued_packets(qemu_get_queue(s->nic));
>>                   }
>
>
> Not familiar with FEC, but if you are tracking 0->1 transition, why not
> simply introduce a parameter of imx_eth_enable_rx() to force the flushing?
>

Not sure I fully understand you, are you proposing I get rid of
"needs_flush" parameter in the device state, converting it to be a
parameter to imx_eth_enable_rx(), and then force flushing every time
imx_eth_enable_rx() is called in imx_eth_write()?

That should work, but it'll end up making the emulator code to flush
corresponding NIC queue every time the driver is done processing RX
ring. If that is not a big problem I am more than happy to make that
change.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings

[Andrey Smirnov]

On Fri, Sep 22, 2017 at 12:33 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年09月19日 03:50, Andrey Smirnov wrote:
>>
>> More recent version of the IP block support more than one Tx DMA ring,
>> so add the code implementing that feature.
>>
>> Cc: Peter Maydell<peter.maydell@linaro.org>
>> Cc: Jason Wang<jasowang@redhat.com>
>> Cc:qemu-devel@nongnu.org
>> Cc:qemu-arm@nongnu.org
>> Cc:yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov<andrew.smirnov@gmail.com>
>> ---
>>   hw/net/imx_fec.c         | 97
>> +++++++++++++++++++++++++++++++++++++++---------
>>   include/hw/net/imx_fec.h | 23 +++++++++++-
>>   2 files changed, 101 insertions(+), 19 deletions(-)
>
>
> Is there a register for driver to know about the version? (Looks like I
> didn't find it).

I haven't seen anything of the sort in the datasheet and, since Linux
driver relies on DT for that information, I am inclined to think
there's none.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length

[Philippe Mathieu-Daudé]

Hi Andrey,

On 09/18/2017 04:50 PM, Andrey Smirnov wrote:
> Frame truncation length, TRUNC_FL, is determined by the contents of
> ENET_FTRL register, so convert the code to use it instead of a
> hardcoded constant.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>   hw/net/imx_fec.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 767402909d..989c11be5f 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>       size += 4;
>   
>       /* Huge frames are truncted.  */
> -    if (size > ENET_MAX_FRAME_SIZE) {
> -        size = ENET_MAX_FRAME_SIZE;
> +    if (size > s->regs[ENET_FTRL]) {
> +        size = s->regs[ENET_FTRL]; >           flags |= ENET_BD_TR | ENET_BD_LG;
>       }

for this to be ok you need to update imx_enet_write(), such:

      case ENET_FTRL:
-        s->regs[index] = value & 0x00003fff;
+        value &= 0x00003fff;
+        if (value > ENET_MAX_FRAME_SIZE) {
+            warn_report("%s: guest requested bigger "
+                        "frame size than QEMU supports "
+                        "(%u > %u)", value,
+                        ENET_MAX_FRAME_SIZE);
+            value = ENET_MAX_FRAME_SIZE;
+        }
+        s->regs[index] = value;
          break;

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH 06/17] imx_fec: Use MIN instead of explicit ternary operator

[Philippe Mathieu-Daudé]

On 09/18/2017 04:50 PM, Andrey Smirnov wrote:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>   hw/net/imx_fec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 989c11be5f..8a77136d38 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1074,7 +1074,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>                             TYPE_IMX_FEC, __func__);
>               break;
>           }
> -        buf_len = (size <= s->regs[ENET_MRBR]) ? size : s->regs[ENET_MRBR];
> +        buf_len = MIN(size, s->regs[ENET_MRBR]);

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>           bd.length = buf_len;
>           size -= buf_len;
>   
> 

Re: [Qemu-devel] [PATCH 01/17] imx_fec: Do not link to netdev

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Binding to a particular netdev doesn't seem to belong to this layer
> and should probably be done as a part of board or SoC specific code.
>
> Convert all of the users of this IP block to use
> qdev_set_nic_properties() instead.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Save some computation time and avoid calculating CRC's frame
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/net/imx_fec.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 88b4b049d7..75822344fc 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1032,9 +1032,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>      IMXENETBufDesc bd;
>      uint32_t flags = 0;
>      uint32_t addr;
> -    uint32_t crc;
>      uint32_t buf_addr;
> -    uint8_t *crc_ptr;
>      unsigned int buf_len;
>      size_t size = len;
>
> @@ -1048,8 +1046,6 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>
>      /* 4 bytes for the CRC.  */
>      size += 4;
> -    crc = cpu_to_be32(crc32(~0, buf, size));
> -    crc_ptr = (uint8_t *) &crc;
>
>      /* Huge frames are truncted.  */
>      if (size > ENET_MAX_FRAME_SIZE) {
> @@ -1090,9 +1086,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>          dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>          buf += buf_len;
>          if (size < 4) {
> +            const uint8_t zeros[4] = { 0 };
> +
>              dma_memory_write(&address_space_memory, buf_addr + buf_len,
> -                             crc_ptr, 4 - size);
> -            crc_ptr += 4 - size;
> +                             zeros, 4 - size);
>          }
>          bd.flags &= ~ENET_BD_E;
>          if (size == 0) {

This looks a bit odd. Doesn't the hardware calculate the CRC here?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/17] imx_fec: Refactor imx_eth_enable_rx()

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Refactor imx_eth_enable_rx() to have more meaningfull variable name
> than 'tmp' and to reduce number of logical negations done.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> In current implementation, packet queue flushing logic seem to suffer
> from a deadlock like scenario if a packet is received by the interface
> before before Rx ring is initialized by Guest's driver. Consider the
> following sequence of events:
>
>         1. A QEMU instance is started against a TAP device on Linux
>            host, running Linux guest, e. g., something to the effect
>            of:
>
>            qemu-system-arm \
>               -net nic,model=imx.fec,netdev=lan0 \
>               netdev tap,id=lan0,ifname=tap0,script=no,downscript=no \
>               ... rest of the arguments ...
>
>         2. Once QEMU starts, but before guest reaches the point where
>            FEC deriver is done initializing the HW, Guest, via TAP
>            interface, receives a number of multicast MDNS packets from
>            Host (not necessarily true for every OS, but it happens at
>            least on Fedora 25)
>
>         3. Recieving a packet in such a state results in
>            imx_eth_can_receive() returning '0', which in turn causes
>            tap_send() to disable corresponding event (tap.c:203)
>
>         4. Once Guest's driver reaches the point where it is ready to
>            recieve packets it prepares Rx ring descriptors and writes
>            ENET_RDAR_RDAR to ENET_RDAR register to indicate to HW that
>            more descriptors are ready. And at this points emulation
>            layer does this:
>
>                  s->regs[index] = ENET_RDAR_RDAR;
>                  imx_eth_enable_rx(s);
>
>            which, combined with:
>
>                   if (!s->regs[ENET_RDAR]) {
>                      qemu_flush_queued_packets(qemu_get_queue(s->nic));
>                   }
>
>            results in Rx queue never being flushed and corresponding
>            I/O event beign disabled.
>
> Change the code to remember the fact that can_receive callback was
> called before Rx ring was ready and use it to make a decision if
> receive queue needs to be flushed.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/net/imx_fec.c         | 6 ++++--
>  include/hw/net/imx_fec.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 84085afe09..767402909d 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -544,8 +544,9 @@ static void imx_eth_enable_rx(IMXFECState *s)
>
>      if (rx_ring_full) {
>          FEC_PRINTF("RX buffer full\n");
> -    } else if (!s->regs[ENET_RDAR]) {
> +    } else if (s->needs_flush) {
>          qemu_flush_queued_packets(qemu_get_queue(s->nic));
> +        s->needs_flush = false;
>      }
>
>      s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
> @@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
>
>      FEC_PRINTF("\n");
>
> -    return s->regs[ENET_RDAR] ? 1 : 0;
> +    s->needs_flush = !s->regs[ENET_RDAR];
> +    return !!s->regs[ENET_RDAR];
>  }
>
>  static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 62ad473b05..4bc8f03ec2 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -252,6 +252,7 @@ typedef struct IMXFECState {
>      uint32_t phy_int_mask;
>
>      bool is_fec;
> +    bool needs_flush;
>  } IMXFECState;

This looks odd -- I don't think you should need extra
state here. Conceptually what you want is:

 * in the can_receive callback, test some function of
various bits of device state to decide whether you can
take data
 * in the rest of the device, whenever the device state
changes such that you were previously not able to take
data but now you can, call qemu_flush_queued_packets().

You shouldn't need any extra state to do this, you just
need to fix the bug where you have a code path that
flips ENET_RDAR from 0 to 1 without calling flush
(you might for instance have a helper function for
"set ENET_RDAR" that encapsulates setting the state
and arranging that flush is called).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length

[Peter Maydell]

On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Andrey,
>
> On 09/18/2017 04:50 PM, Andrey Smirnov wrote:
>>
>> Frame truncation length, TRUNC_FL, is determined by the contents of
>> ENET_FTRL register, so convert the code to use it instead of a
>> hardcoded constant.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>   hw/net/imx_fec.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 767402909d..989c11be5f 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc,
>> const uint8_t *buf,
>>       size += 4;
>>         /* Huge frames are truncted.  */
>> -    if (size > ENET_MAX_FRAME_SIZE) {
>> -        size = ENET_MAX_FRAME_SIZE;
>> +    if (size > s->regs[ENET_FTRL]) {
>> +        size = s->regs[ENET_FTRL]; >           flags |= ENET_BD_TR |
>> ENET_BD_LG;
>>       }
>
>
> for this to be ok you need to update imx_enet_write(), such:
>
>      case ENET_FTRL:
> -        s->regs[index] = value & 0x00003fff;
> +        value &= 0x00003fff;
> +        if (value > ENET_MAX_FRAME_SIZE) {
> +            warn_report("%s: guest requested bigger "
> +                        "frame size than QEMU supports "
> +                        "(%u > %u)", value,
> +                        ENET_MAX_FRAME_SIZE);
> +            value = ENET_MAX_FRAME_SIZE;
> +        }
> +        s->regs[index] = value;
>          break;

Yes, and also an incoming-migration post_load callback that
fails migration if the value is too large.

It might be simpler to truncate to the smaller of the ENET_FTRL
register value and ENET_MAX_FRAME_SIZE.

(PS: what is the hardware behaviour if ENET_FTRL is set to
a larger value than ENET_MAX_FRAME_SIZE ?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Needed to support latest Linux kernel driver which relies on that
> functionality.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/net/imx_fec.c         | 23 +++++++++++++++++++++++
>  include/hw/net/imx_fec.h |  2 ++
>  2 files changed, 25 insertions(+)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 8a77136d38..bd62d7a75f 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1037,6 +1037,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>      uint32_t buf_addr;
>      unsigned int buf_len;
>      size_t size = len;
> +    bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
>
>      FEC_PRINTF("len %d\n", (int)size);
>
> @@ -1049,6 +1050,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>      /* 4 bytes for the CRC.  */
>      size += 4;
>
> +    if (shift16) {
> +        size += 2;
> +    }
> +
>      /* Huge frames are truncted.  */
>      if (size > s->regs[ENET_FTRL]) {
>          size = s->regs[ENET_FTRL];
> @@ -1085,6 +1090,24 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>              buf_len += size - 4;
>          }
>          buf_addr = bd.data;
> +
> +        if (shift16) {
> +            /*
> +             * If SHIFT16 bit of ENETx_RACC register is set we need to
> +             * align the payload to 4-byte boundary.
> +             */
> +            const uint8_t zeros[2] = { 0 };
> +
> +            dma_memory_write(&address_space_memory, buf_addr,
> +                             zeros, sizeof(zeros));
> +
> +            buf_addr += sizeof(zeros);
> +            buf_len  -= sizeof(zeros);
> +
> +            shift16 = false; /* We only do this once per Ethernet
> +                              * frame */
> +        }
> +

Can you avoid having an end-of-source-line comment that wraps
to multiple lines, please? (put it on a line of its own, or
edit down to fit.) Otherwise

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Frame truncation length, TRUNC_FL, is determined by the contents of
> ENET_FTRL register, so convert the code to use it instead of a
> hardcoded constant.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/net/imx_fec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 767402909d..989c11be5f 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>      size += 4;
>
>      /* Huge frames are truncted.  */
> -    if (size > ENET_MAX_FRAME_SIZE) {
> -        size = ENET_MAX_FRAME_SIZE;
> +    if (size > s->regs[ENET_FTRL]) {
> +        size = s->regs[ENET_FTRL];
>          flags |= ENET_BD_TR | ENET_BD_LG;
>      }

PS: since you're editing this bit of code anyway, can you fix
the typo in the comment in passing: should be "truncated".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> More recent version of the IP block support more than one Tx DMA ring,
> so add the code implementing that feature.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

I'm surprised this doesn't mean "we have a property on the device
to indicate what IP version it is, which the boards set". Or are
all our current boards mismodelling their ethernet devices with the
wrong number of TX rings ?

> ---
>  hw/net/imx_fec.c         | 97 +++++++++++++++++++++++++++++++++++++++---------
>  include/hw/net/imx_fec.h | 23 +++++++++++-
>  2 files changed, 101 insertions(+), 19 deletions(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index bd62d7a75f..6045ffe673 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -196,6 +196,17 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
>      }
>  }
>
> +static const VMStateDescription vmstate_imx_eth_tx_ring = {
> +    .name = "fec-tx-ring",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(descriptor, IMXFECTxRing),
> +        VMSTATE_UINT32(tdsr, IMXFECTxRing),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static const VMStateDescription vmstate_imx_eth = {
>      .name = TYPE_IMX_FEC,
>      .version_id = 2,
> @@ -203,8 +214,10 @@ static const VMStateDescription vmstate_imx_eth = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32_ARRAY(regs, IMXFECState, ENET_MAX),
>          VMSTATE_UINT32(rx_descriptor, IMXFECState),
> -        VMSTATE_UINT32(tx_descriptor, IMXFECState),
> -
> +        VMSTATE_STRUCT_ARRAY(tx_ring, IMXFECState,
> +                             ENET_TX_RING_NUM,
> +                             1, vmstate_imx_eth_tx_ring,
> +                             IMXFECTxRing),
>          VMSTATE_UINT32(phy_status, IMXFECState),
>          VMSTATE_UINT32(phy_control, IMXFECState),
>          VMSTATE_UINT32(phy_advertise, IMXFECState),

This breaks migration compatibility, so you need to figure out
how you want to handle that. The simple solution is to increment
the version_id and minimum_version_id fields in vmstate_imx_eth;
that will mean migration from an old QEMU to a new one doesn't
work but it fails in a clean way. The complex way is to put the
new state into a migration subsection with a .needed function
to determine whether to use it or not. For imx I think we can
get away with the simple solution.

> @@ -407,7 +420,7 @@ static void imx_fec_do_tx(IMXFECState *s)
>      int frame_size = 0, descnt = 0;
>      uint8_t frame[ENET_MAX_FRAME_SIZE];
>      uint8_t *ptr = frame;
> -    uint32_t addr = s->tx_descriptor;
> +    uint32_t addr = s->tx_ring[0].descriptor;
>
>      while (descnt++ < IMX_MAX_DESC) {
>          IMXFECBufDesc bd;
> @@ -448,17 +461,38 @@ static void imx_fec_do_tx(IMXFECState *s)
>          }
>      }
>
> -    s->tx_descriptor = addr;
> +    s->tx_ring[0].descriptor = addr;
>
>      imx_eth_update(s);
>  }
>
> -static void imx_enet_do_tx(IMXFECState *s)
> +static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
>  {
>      int frame_size = 0, descnt = 0;
>      uint8_t frame[ENET_MAX_FRAME_SIZE];
>      uint8_t *ptr = frame;
> -    uint32_t addr = s->tx_descriptor;
> +    IMXFECTxRing *ring;
> +    uint32_t addr;
> +
> +    switch (index) {
> +    case ENET_TDAR:
> +        ring = &s->tx_ring[0];
> +        break;
> +    case ENET_TDAR1:
> +        ring = &s->tx_ring[1];
> +        break;
> +    case ENET_TDAR2:
> +        ring = &s->tx_ring[2];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: bogus value for index %x\n",
> +                      __func__, index);
> +        abort();
> +        break;
> +    }
> +
> +    addr = ring->descriptor;
>
>      while (descnt++ < IMX_MAX_DESC) {
>          IMXENETBufDesc bd;
> @@ -502,32 +536,32 @@ static void imx_enet_do_tx(IMXFECState *s)
>              ptr = frame;
>              frame_size = 0;
>              if (bd.option & ENET_BD_TX_INT) {
> -                s->regs[ENET_EIR] |= ENET_INT_TXF;
> +                s->regs[ENET_EIR] |= ring->int_txf;
>              }
>          }
>          if (bd.option & ENET_BD_TX_INT) {
> -            s->regs[ENET_EIR] |= ENET_INT_TXB;
> +            s->regs[ENET_EIR] |= ring->int_txb;
>          }
>          bd.flags &= ~ENET_BD_R;
>          /* Write back the modified descriptor.  */
>          imx_enet_write_bd(&bd, addr);
>          /* Advance to the next descriptor.  */
>          if ((bd.flags & ENET_BD_W) != 0) {
> -            addr = s->regs[ENET_TDSR];
> +            addr = s->regs[ring->tdsr];
>          } else {
>              addr += sizeof(bd);
>          }
>      }
>
> -    s->tx_descriptor = addr;
> +    ring->descriptor = addr;
>
>      imx_eth_update(s);
>  }
>
> -static void imx_eth_do_tx(IMXFECState *s)
> +static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
>  {
>      if (!s->is_fec && (s->regs[ENET_ECR] & ENET_ECR_EN1588)) {
> -        imx_enet_do_tx(s);
> +        imx_enet_do_tx(s, index);
>      } else {
>          imx_fec_do_tx(s);
>      }
> @@ -586,7 +620,22 @@ static void imx_eth_reset(DeviceState *d)
>      }
>
>      s->rx_descriptor = 0;
> -    s->tx_descriptor = 0;
> +
> +    s->tx_ring[0].tdsr = ENET_TDSR;
> +    s->tx_ring[1].tdsr = ENET_TDSR1;
> +    s->tx_ring[2].tdsr = ENET_TDSR2;
> +
> +    s->tx_ring[0].int_txf = ENET_INT_TXF;
> +    s->tx_ring[1].int_txf = ENET_INT_TXF1;
> +    s->tx_ring[2].int_txf = ENET_INT_TXF2;
> +
> +    s->tx_ring[0].int_txb = ENET_INT_TXB;
> +    s->tx_ring[1].int_txb = ENET_INT_TXB1;
> +    s->tx_ring[2].int_txb = ENET_INT_TXB2;
> +
> +    s->tx_ring[0].descriptor = 0;
> +    s->tx_ring[1].descriptor = 0;
> +    s->tx_ring[2].descriptor = 0;
>
>      /* We also reset the PHY */
>      phy_reset(s);
> @@ -814,10 +863,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>              s->regs[index] = 0;
>          }
>          break;
> +    case ENET_TDAR1:    /* FALLTHROUGH */
> +    case ENET_TDAR2:    /* FALLTHROUGH */
>      case ENET_TDAR:
>          if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>              s->regs[index] = ENET_TDAR_TDAR;
> -            imx_eth_do_tx(s);
> +            imx_eth_do_tx(s, index);
>          }
>          s->regs[index] = 0;
>          break;
> @@ -829,8 +880,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>          if ((s->regs[index] & ENET_ECR_ETHEREN) == 0) {
>              s->regs[ENET_RDAR] = 0;
>              s->rx_descriptor = s->regs[ENET_RDSR];
> -            s->regs[ENET_TDAR] = 0;
> -            s->tx_descriptor = s->regs[ENET_TDSR];
> +            s->regs[ENET_TDAR]  = 0;
> +            s->regs[ENET_TDAR1] = 0;
> +            s->regs[ENET_TDAR2] = 0;
> +            s->tx_ring[0].descriptor = s->regs[ENET_TDSR];
> +            s->tx_ring[1].descriptor = s->regs[ENET_TDSR1];
> +            s->tx_ring[2].descriptor = s->regs[ENET_TDSR2];
>          }
>          break;
>      case ENET_MMFR:
> @@ -908,7 +963,15 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>          } else {
>              s->regs[index] = value & ~7;
>          }
> -        s->tx_descriptor = s->regs[index];
> +        s->tx_ring[0].descriptor = s->regs[index];
> +        break;
> +    case ENET_TDSR1:
> +        s->regs[index] = value & ~7;
> +        s->tx_ring[1].descriptor = s->regs[index];
> +        break;
> +    case ENET_TDSR2:
> +        s->regs[index] = value & ~7;
> +        s->tx_ring[2].descriptor = s->regs[index];
>          break;
>      case ENET_MRBR:
>          s->regs[index] = value & 0x00003ff0;
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 20a6aa98b4..40bd29771f 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -52,6 +52,8 @@
>  #define ENET_TFWR              81
>  #define ENET_FRBR              83
>  #define ENET_FRSR              84
> +#define ENET_TDSR1             89
> +#define ENET_TDSR2             92
>  #define ENET_RDSR              96
>  #define ENET_TDSR              97
>  #define ENET_MRBR              98
> @@ -66,6 +68,8 @@
>  #define ENET_FTRL              108
>  #define ENET_TACC              112
>  #define ENET_RACC              113
> +#define ENET_TDAR1             121
> +#define ENET_TDAR2             123
>  #define ENET_MIIGSK_CFGR       192
>  #define ENET_MIIGSK_ENR        194
>  #define ENET_ATCR              256
> @@ -106,13 +110,18 @@
>  #define ENET_INT_WAKEUP        (1 << 17)
>  #define ENET_INT_TS_AVAIL      (1 << 16)
>  #define ENET_INT_TS_TIMER      (1 << 15)
> +#define ENET_INT_TXF2          (1 <<  7)
> +#define ENET_INT_TXB2          (1 <<  6)
> +#define ENET_INT_TXF1          (1 <<  3)
> +#define ENET_INT_TXB1          (1 <<  2)
>
>  #define ENET_INT_MAC           (ENET_INT_HB | ENET_INT_BABR | ENET_INT_BABT | \
>                                  ENET_INT_GRA | ENET_INT_TXF | ENET_INT_TXB | \
>                                  ENET_INT_RXF | ENET_INT_RXB | ENET_INT_MII | \
>                                  ENET_INT_EBERR | ENET_INT_LC | ENET_INT_RL | \
>                                  ENET_INT_UN | ENET_INT_PLR | ENET_INT_WAKEUP | \
> -                                ENET_INT_TS_AVAIL)
> +                                ENET_INT_TS_AVAIL | ENET_INT_TXF1 | ENET_INT_TXB1 |\
> +                                ENET_INT_TXF2 | ENET_INT_TXB2)
>
>  /* RDAR */
>  #define ENET_RDAR_RDAR         (1 << 24)
> @@ -233,6 +242,15 @@ typedef struct {
>
>  #define ENET_BD_BDU            (1 << 31)
>
> +#define ENET_TX_RING_NUM       3
> +
> +typedef struct IMXFECTxRing {
> +    uint32_t descriptor;
> +    uint32_t tdsr;
> +    uint32_t int_txf;
> +    uint32_t int_txb;

Why aren't the int_txf and int_txb fields in the migration state?
What are they for? It looks from the code like they're just constants
indicating the tx interrupt bits to use for this tx ring, in which
case they'd be better implemented using a utility function which
returns the tx bit when given a ring number, I think.

> +} IMXFECTxRing;
> +
>  typedef struct IMXFECState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -245,7 +263,8 @@ typedef struct IMXFECState {
>
>      uint32_t regs[ENET_MAX];
>      uint32_t rx_descriptor;
> -    uint32_t tx_descriptor;
> +
> +    IMXFECTxRing tx_ring[ENET_TX_RING_NUM];
>
>      uint32_t phy_status;
>      uint32_t phy_control;
> --
> 2.13.5


thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/17] imx_fec: Use correct length for packet size

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Use 'frame_size' instead of 'len' when calling qemu_send_packet(),
> failing to do so results in malformed packets send in case when that
> packed is fragmented into multiple DMA transactions.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/net/imx_fec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index 6045ffe673..c45b9648d9 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -532,7 +532,7 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
>                  }
>              }
>              /* Last buffer in frame.  */
> -            qemu_send_packet(qemu_get_queue(s->nic), frame, len);
> +            qemu_send_packet(qemu_get_queue(s->nic), frame, frame_size);
>              ptr = frame;
>              frame_size = 0;
>              if (bd.option & ENET_BD_TX_INT) {

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Add sizes.h from Linux to have a more readable way of specifying
> MemoryRegion sizes.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  include/qemu/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 include/qemu/sizes.h
>
> diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
> new file mode 100644
> index 0000000000..9aedb9f8f6
> --- /dev/null
> +++ b/include/qemu/sizes.h
> @@ -0,0 +1,47 @@
> +/*
> + * Copy of include/linux/sizes.h
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */

Not really convinced of the utility, and it's gpl-2-only, so no thanks.

-- PMM

Re: [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Add enough code to emulate i.MX2 watchdog IP block so it would be
> possible to reboot the machine running Linux Guest.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/misc/Makefile.objs      |   1 +
>  hw/misc/imx2_wdt.c         | 117 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/imx2_wdt.h |  36 ++++++++++++++
>  3 files changed, 154 insertions(+)
>  create mode 100644 hw/misc/imx2_wdt.c
>  create mode 100644 include/hw/misc/imx2_wdt.h
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index ac1be05a03..c393a93456 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx25_ccm.o
>  obj-$(CONFIG_IMX) += imx6_ccm.o
>  obj-$(CONFIG_IMX) += imx6_src.o
>  obj-$(CONFIG_IMX) += imx7_ccm.o
> +obj-$(CONFIG_IMX) += imx2_wdt.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>  obj-$(CONFIG_MAINSTONE) += mst_fpga.o
> diff --git a/hw/misc/imx2_wdt.c b/hw/misc/imx2_wdt.c
> new file mode 100644
> index 0000000000..9d97a19511
> --- /dev/null
> +++ b/hw/misc/imx2_wdt.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * i.MX2 Watchdog IP block
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * 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 "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/hw.h"
> +#include "exec/memory.h"
> +#include "exec/address-spaces.h"
> +#include "hw/sysbus.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/watchdog.h"
> +#include "qemu/error-report.h"
> +#include "qemu/sizes.h"

That's an awful lot of includes for a very simple device. Are
you sure they're all needed?

> +
> +#include "hw/misc/imx2_wdt.h"
> +
> +
> +#define IMX2_WDT_WCR_WDA       BIT(5)          /* -> External Reset WDOG_B */
> +#define IMX2_WDT_WCR_SRS       BIT(4)          /* -> Software Reset Signal */
> +
> +static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
> +                              unsigned int size)
> +{
> +    IMX2WdtState *s = opaque;
> +    const size_t index = addr / sizeof(s->reg[0]);
> +
> +    if (index < ARRAY_SIZE(s->reg))
> +        return s->reg[index];
> +    else
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +
> +    return 0xDEADBEEF;
> +}
> +
> +static void imx2_wdt_write(void *opaque, hwaddr addr,
> +                           uint64_t val64, unsigned int size)
> +{
> +    uint16_t value  = val64;
> +    IMX2WdtState *s = opaque;
> +    const size_t index = addr / sizeof(s->reg[0]);
> +
> +    switch (index) {
> +    case IMX2_WDT_WCR:
> +        if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
> +            watchdog_perform_action();

Missing "break"?

Also checkpatch should tell you you need more braces.

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
> +    }
> +}
> +
> +static const MemoryRegionOps imx2_wdt_ops = {
> +    .read  = imx2_wdt_read,
> +    .write = imx2_wdt_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.
> +         */

This sounds like it's perhaps a job for .impl.unaligned
rather than .valid.unaligned ?

> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +        .unaligned = false,
> +    },
> +};
> +
> +static void imx2_wdt_realize(DeviceState *dev, Error **errp)
> +{
> +    IMX2WdtState *s = IMX2_WDT(dev);
> +
> +    memory_region_init_io(&s->mmio, OBJECT(dev),
> +                          &imx2_wdt_ops, s,
> +                          TYPE_IMX2_WDT".mmio", SZ_64K);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
> +}
> +
> +static void imx2_wdt_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = imx2_wdt_realize;
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo imx2_wdt_info = {
> +    .name          = TYPE_IMX2_WDT,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IMX2WdtState),
> +    .class_init    = imx2_wdt_class_init,
> +};

This device needs a reset function and vmstate description.

> +
> +static WatchdogTimerModel model = {
> +    .wdt_name = "imx2-watchdog",
> +    .wdt_description = "i.MX2 Watchdog",
> +};
> +
> +static void imx2_wdt_register_type(void)
> +{
> +    watchdog_add_model(&model);
> +    type_register_static(&imx2_wdt_info);
> +}

thanks
-- PMM

Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> For now we only support the following devices:
>     * up to 2 Cortex A9 cores (SMP works with PSCI)
>     * A7 MPCORE (identical to A15 MPCORE)
>     * 7 i.MX UARTs
>     * 1 CCM device
>     * 2 Ethernet controllers (FEC)
>     * 3 SD controllers (USDHC)
>     * 1 SNVS device
>     * 1 WDT device
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/arm/Makefile.objs            |   2 +
>  hw/arm/fsl-imx7.c               | 327 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx7.h       | 114 ++++++++++++++
>  4 files changed, 444 insertions(+)
>  create mode 100644 hw/arm/fsl-imx7.c
>  create mode 100644 include/hw/arm/fsl-imx7.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index bbdd3c1d8b..98396a3ad2 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
>  CONFIG_FSL_IMX6=y
>  CONFIG_FSL_IMX31=y
>  CONFIG_FSL_IMX25=y
> +CONFIG_FSL_IMX7=y
>
>  CONFIG_IMX_I2C=y
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index a2e56ecaae..33f6051ae3 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>  obj-$(CONFIG_MPS2) += mps2.o
> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
> +
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> new file mode 100644
> index 0000000000..bd01bb7f59
> --- /dev/null
> +++ b/hw/arm/fsl-imx7.c
> @@ -0,0 +1,327 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * i.MX7 SoC definitions
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * Based on hw/arm/fsl-imx6.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.

We pretty strongly prefer GPL-2-or-later for new code, not 2-only.

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/arm/fsl-imx7.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +#define NAME_SIZE 20
> +
> +#define for_each_cpu(i)     for (i = 0; i < smp_cpus; i++)

Just open-code this, please.

> +
> +static void fsl_imx7_init(Object *obj)
> +{
> +    BusState *sysbus = sysbus_get_default();
> +    FslIMX7State *s = FSL_IMX7(obj);
> +    char name[NAME_SIZE];
> +    int i;
> +
> +    if (smp_cpus > FSL_IMX7_NUM_CPUS) {
> +        error_report("%s: Only %d CPUs are supported (%d requested)",
> +                     TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
> +        exit(1);
> +    }
> +
> +    for_each_cpu(i) {
> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +                          "cortex-a7-" TYPE_ARM_CPU);
> +        snprintf(name, NAME_SIZE, "cpu%d", i);
> +        object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
> +                                  &error_fatal);
> +    }
> +
> +    /*
> +     * A7MPCORE
> +     */
> +    object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);

Stealing the A15MPCORE device for a7 is kind of ugly, but I can't
decide what would be better instead...

> +    qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
> +    object_property_add_child(obj, "a7mpcore",
> +                              OBJECT(&s->a7mpcore), &error_fatal);
> +
> +    /*
> +     * CCM
> +     */
> +    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
> +    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
> +    object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
> +
> +    /*
> +     * UART
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
> +            object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
> +            qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "uart%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->uart[i]),
> +                                      &error_fatal);
> +    }
> +
> +    /*
> +     * Ethernet
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
> +            object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
> +            qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "eth%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->eth[i]),
> +                                      &error_fatal);
> +    }
> +
> +    /*
> +     * SDHCI
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> +            object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
> +                              TYPE_IMX_USDHC);
> +            qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "usdhc%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
> +                                      &error_fatal);
> +    }
> +
> +    /*
> +     * SNVS
> +     */
> +    object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
> +    qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
> +    object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
> +
> +    /*
> +     * Watchdog
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
> +            object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
> +            qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "wdt%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
> +                                      &error_fatal);
> +    }
> +}
> +
> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
> +{
> +    FslIMX7State *s = FSL_IMX7(dev);
> +    Object *o;
> +    int i;
> +    qemu_irq irq;
> +
> +    for_each_cpu(i) {
> +       o = OBJECT(&s->cpu[i]);
> +
> +        object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
> +                                "psci-conduit", &error_abort);
> +
> +        object_property_set_bool(o, false, "has_el3", &error_abort);

Does this SoC's CPU really not have EL3?

> +
> +        /* On uniprocessor, the CBAR is set to 0 */
> +        if (smp_cpus > 1) {
> +            object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
> +                                    "reset-cbar", &error_abort);
> +        }
> +
> +        if (i) {
> +            /* Secondary CPUs start in PSCI powered-down state */
> +            object_property_set_bool(o, true,
> +                                     "start-powered-off", &error_abort);
> +        }
> +
> +        object_property_set_bool(o, true, "realized", &error_abort);
> +    }
> +
> +    /*
> +     * A7MPCORE
> +     */
> +    object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->a7mpcore),
> +                            FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
> +                            "num-irq", &error_abort);
> +
> +    object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
> +                             &error_abort);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
> +
> +    for_each_cpu(i) {
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
> +        DeviceState  *d   = DEVICE(qemu_get_cpu(i));
> +
> +        irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
> +        sysbus_connect_irq(sbd, i, irq);
> +        irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
> +        sysbus_connect_irq(sbd, i + smp_cpus, irq);
> +    }
> +
> +    /*
> +     * CCM
> +     */
> +    object_property_set_bool(OBJECT(&s->ccm), true, "realized", &error_abort);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
> +
> +    /*
> +     * UART
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
> +        static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
> +            FSL_IMX7_UART1_ADDR,
> +            FSL_IMX7_UART2_ADDR,
> +            FSL_IMX7_UART3_ADDR,
> +            FSL_IMX7_UART4_ADDR,
> +            FSL_IMX7_UART5_ADDR,
> +            FSL_IMX7_UART6_ADDR,
> +            FSL_IMX7_UART7_ADDR,
> +        };
> +
> +        static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
> +            FSL_IMX7_UART1_IRQ,
> +            FSL_IMX7_UART2_IRQ,
> +            FSL_IMX7_UART3_IRQ,
> +            FSL_IMX7_UART4_IRQ,
> +            FSL_IMX7_UART5_IRQ,
> +            FSL_IMX7_UART6_IRQ,
> +            FSL_IMX7_UART7_IRQ,
> +        };
> +
> +
> +        if (i < MAX_SERIAL_PORTS) {
> +            Chardev *chr;
> +            chr = serial_hds[i];
> +
> +            if (!chr) {
> +                char *label = g_strdup_printf("imx7.uart%d", i + 1);
> +                chr = qemu_chr_new(label, "null");
> +                g_free(label);
> +                serial_hds[i] = chr;
> +            }

I think the conclusion we've come to about chardevs is that the
UART should handle being passed a NULL chardev, rather than the
board code having to create a dummy chardev to pass to it.

> +
> +            qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> +        }
> +
> +        object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, FSL_IMX7_UARTn_ADDR[i]);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
> +    }
> +
> +    /*
> +     * Etherenet

Typo: "Ethernet"

> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
> +        static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
> +            FSL_IMX7_ENET1_ADDR,
> +            FSL_IMX7_ENET2_ADDR,
> +        };
> +
> +        qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
> +        object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 0));
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 3));
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
> +    }
> +
> +    /*
> +     * USDHC
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> +        static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
> +            FSL_IMX7_USDHC1_ADDR,
> +            FSL_IMX7_USDHC2_ADDR,
> +            FSL_IMX7_USDHC3_ADDR,
> +        };
> +
> +        static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
> +            FSL_IMX7_USDHC1_IRQ,
> +            FSL_IMX7_USDHC2_IRQ,
> +            FSL_IMX7_USDHC3_IRQ,
> +        };
> +
> +        object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
> +                        FSL_IMX7_USDHCn_ADDR[i]);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
> +    }
> +
> +    /*
> +     * SNVS
> +     */
> +    object_property_set_bool(OBJECT(&s->snvs), true, "realized", &error_abort);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
> +
> +    /*
> +     * Watchdog
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
> +        static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
> +            FSL_IMX7_WDOG1_ADDR,
> +            FSL_IMX7_WDOG2_ADDR,
> +            FSL_IMX7_WDOG3_ADDR,
> +            FSL_IMX7_WDOG4_ADDR,
> +        };
> +
> +        object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, FSL_IMX7_WDOGn_ADDR[i]);
> +    }
> +}
> +
> +static void fsl_imx7_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = fsl_imx7_realize;
> +
> +    /*
> +     * Reason: creates an ARM CPU, thus use after free(), see
> +     * arm_cpu_class_init()
> +     */

I think this is the wrong reason (we've fixed that problem, IIRC).
You do want this not to be user-creatable, but the reason is
/* Reason: uses serial_hds and nd_table in realize() */
(compare the reason text in the aspeed/allwinner/digic/xlnx-zynqmp
SoC device objects).

> +    dc->user_creatable = false;
> +    dc->desc = "i.MX7 SOC";
> +}
> +
> +static const TypeInfo fsl_imx7_type_info = {
> +    .name = TYPE_FSL_IMX7,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(FslIMX7State),
> +    .instance_init = fsl_imx7_init,
> +    .class_init = fsl_imx7_class_init,
> +};
> +
> +static void fsl_imx7_register_types(void)
> +{
> +    type_register_static(&fsl_imx7_type_info);
> +}
> +type_init(fsl_imx7_register_types)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board

[Peter Maydell]

On 18 September 2017 at 20:51, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Cc: qemu-arm@nongnu.org
> Cc: yurovsky@gmail.com
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  hw/arm/Makefile.objs   |   2 +-
>  hw/arm/mcimx7d-sabre.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/mcimx7d-sabre.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 33f6051ae3..fc4a963de8 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>  obj-$(CONFIG_MPS2) += mps2.o
> -obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
>
> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
> new file mode 100644
> index 0000000000..34e3933db8
> --- /dev/null
> +++ b/hw/arm/mcimx7d-sabre.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * MCIMX7D_SABRE Board System emulation.
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * This code is licensed under the GPL, version 2 or later.
> + * See the file `COPYING' in the top level directory.
> + *
> + * It (partially) emulates a mcimx7d_sabre board, with a Freescale
> + * i.MX7 SoC
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/arm/fsl-imx7.h"
> +#include "hw/boards.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/device_tree.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/qtest.h"
> +#include "net/net.h"
> +
> +typedef struct {
> +    FslIMX7State soc;
> +    MemoryRegion ram;
> +} MCIMX7Sabre;
> +
> +static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
> +{
> +    const char comp[] = "arm,psci-0.2\0arm,psci";
> +
> +    qemu_fdt_add_subnode(fdt, "/psci");
> +    qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
> +    qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
> +}

You shouldn't need this -- we should just be able to work with whatever
the real hardware's device tree is. ("virt" is a special case because
we create our own device tree there.)

> +
> +static void mcimx7d_sabre_init(MachineState *machine)
> +{
> +    static struct arm_boot_info boot_info;
> +    MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
> +    Object *soc;
> +    int i;
> +
> +    if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
> +        error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
> +                     machine->ram_size, FSL_IMX7_MMDC_SIZE);
> +        exit(1);
> +    }
> +
> +    boot_info = (struct arm_boot_info) {
> +        .loader_start = FSL_IMX7_MMDC_ADDR,
> +        .board_id = -1,
> +        .ram_size = machine->ram_size,
> +        .kernel_filename = machine->kernel_filename,
> +        .kernel_cmdline = machine->kernel_cmdline,
> +        .initrd_filename = machine->initrd_filename,
> +        .nb_cpus = smp_cpus,
> +        .modify_dtb = mcimx7d_add_psci_node,
> +    };
> +
> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
> +    soc = OBJECT(&s->soc);
> +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
> +    object_property_set_bool(soc, true, "realized", &error_fatal);
> +
> +    memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
> +                                         machine->ram_size);
> +    memory_region_add_subregion(get_system_memory(),
> +                                FSL_IMX7_MMDC_ADDR, &s->ram);
> +
> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> +        BusState *bus;
> +        DeviceState *carddev;
> +        DriveInfo *di;
> +        BlockBackend *blk;
> +
> +        di = drive_get_next(IF_SD);
> +        blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +        bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
> +        carddev = qdev_create(bus, TYPE_SD_CARD);
> +        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> +        object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
> +    }
> +
> +    if (!qtest_enabled()) {
> +        arm_load_kernel(&s->soc.cpu[0], &boot_info);
> +    }
> +}
> +
> +static void mcimx7d_sabre_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
> +    mc->init = mcimx7d_sabre_init;
> +    mc->max_cpus = FSL_IMX7_NUM_CPUS;
> +    mc->ignore_memory_transaction_failures = true;

Please don't set this flag on new board models -- it is only
for legacy existing board models (where we don't know what
running guest code might be broken by turning bad accesses
into guest aborts. For a new board model, you should leave
the flag unset and instead stub out any devices that you
need to using create_unimplemented_device() so that your
guest code boots.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/17] Initial i.MX7 support

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> Hi everyone,
>
> This patch series contains the work that I've done in order to enable
> support for i.MX7 emulation in QEMU. Majority of the set are just odd
> fixes and small features implementation that I had to do to already
> exisitng code but last 5 commits contain new emulation code.
>
> As the one before last commit in the series states the supported i.MX7
> features are:
>
>     * up to 2 Cortex A9 cores (SMP works with PSCI)
>     * A7 MPCORE (identical to A15 MPCORE)
>     * 7 i.MX UARTs
>     * 1 CCM device
>     * 2 Ethernet controllers (FEC)
>     * 3 SD controllers (USDHC)
>     * 1 SNVS device
>     * 1 WDT device
>
> I also have a follow up series that implements bit needes for PCIe
> emulation support (DesignWare IP emulation + supporting code) which
> I'll be submitting after this series is accepted.

Hi; thanks for these patches. I've now reviewed most of them. Hopefully
somebody else can get to the sdhci patches, as sdhci is not an
area I know much about and I don't think I have the specs.

Please also run your patches through scripts/checkpatch.pl and fix
those issues as necessary (the script does occasionally have false
positives so you need not follow it blindly if it suggests something
ridiculous).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 6:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Save some computation time and avoid calculating CRC's frame
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  hw/net/imx_fec.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 88b4b049d7..75822344fc 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1032,9 +1032,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>      IMXENETBufDesc bd;
>>      uint32_t flags = 0;
>>      uint32_t addr;
>> -    uint32_t crc;
>>      uint32_t buf_addr;
>> -    uint8_t *crc_ptr;
>>      unsigned int buf_len;
>>      size_t size = len;
>>
>> @@ -1048,8 +1046,6 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>
>>      /* 4 bytes for the CRC.  */
>>      size += 4;
>> -    crc = cpu_to_be32(crc32(~0, buf, size));
>> -    crc_ptr = (uint8_t *) &crc;
>>
>>      /* Huge frames are truncted.  */
>>      if (size > ENET_MAX_FRAME_SIZE) {
>> @@ -1090,9 +1086,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>          dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>>          buf += buf_len;
>>          if (size < 4) {
>> +            const uint8_t zeros[4] = { 0 };
>> +
>>              dma_memory_write(&address_space_memory, buf_addr + buf_len,
>> -                             crc_ptr, 4 - size);
>> -            crc_ptr += 4 - size;
>> +                             zeros, 4 - size);
>>          }
>>          bd.flags &= ~ENET_BD_E;
>>          if (size == 0) {
>
> This looks a bit odd. Doesn't the hardware calculate the CRC here?
>

It does, it just seemed to me that since the hardware also has a "CRC
error" bit in its status register, there would be few if any users of
the actual calculated CRC value. Given how eTSEC emulation layer gets
away without calculating CRC I thought that it might be possible to
have this optimization here as well.

I can drop this patch if this seems risky.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 04/17] imx_fec: Change queue flushing heuristics

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 6:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> In current implementation, packet queue flushing logic seem to suffer
>> from a deadlock like scenario if a packet is received by the interface
>> before before Rx ring is initialized by Guest's driver. Consider the
>> following sequence of events:
>>
>>         1. A QEMU instance is started against a TAP device on Linux
>>            host, running Linux guest, e. g., something to the effect
>>            of:
>>
>>            qemu-system-arm \
>>               -net nic,model=imx.fec,netdev=lan0 \
>>               netdev tap,id=lan0,ifname=tap0,script=no,downscript=no \
>>               ... rest of the arguments ...
>>
>>         2. Once QEMU starts, but before guest reaches the point where
>>            FEC deriver is done initializing the HW, Guest, via TAP
>>            interface, receives a number of multicast MDNS packets from
>>            Host (not necessarily true for every OS, but it happens at
>>            least on Fedora 25)
>>
>>         3. Recieving a packet in such a state results in
>>            imx_eth_can_receive() returning '0', which in turn causes
>>            tap_send() to disable corresponding event (tap.c:203)
>>
>>         4. Once Guest's driver reaches the point where it is ready to
>>            recieve packets it prepares Rx ring descriptors and writes
>>            ENET_RDAR_RDAR to ENET_RDAR register to indicate to HW that
>>            more descriptors are ready. And at this points emulation
>>            layer does this:
>>
>>                  s->regs[index] = ENET_RDAR_RDAR;
>>                  imx_eth_enable_rx(s);
>>
>>            which, combined with:
>>
>>                   if (!s->regs[ENET_RDAR]) {
>>                      qemu_flush_queued_packets(qemu_get_queue(s->nic));
>>                   }
>>
>>            results in Rx queue never being flushed and corresponding
>>            I/O event beign disabled.
>>
>> Change the code to remember the fact that can_receive callback was
>> called before Rx ring was ready and use it to make a decision if
>> receive queue needs to be flushed.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  hw/net/imx_fec.c         | 6 ++++--
>>  include/hw/net/imx_fec.h | 1 +
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 84085afe09..767402909d 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -544,8 +544,9 @@ static void imx_eth_enable_rx(IMXFECState *s)
>>
>>      if (rx_ring_full) {
>>          FEC_PRINTF("RX buffer full\n");
>> -    } else if (!s->regs[ENET_RDAR]) {
>> +    } else if (s->needs_flush) {
>>          qemu_flush_queued_packets(qemu_get_queue(s->nic));
>> +        s->needs_flush = false;
>>      }
>>
>>      s->regs[ENET_RDAR] = rx_ring_full ? 0 : ENET_RDAR_RDAR;
>> @@ -930,7 +931,8 @@ static int imx_eth_can_receive(NetClientState *nc)
>>
>>      FEC_PRINTF("\n");
>>
>> -    return s->regs[ENET_RDAR] ? 1 : 0;
>> +    s->needs_flush = !s->regs[ENET_RDAR];
>> +    return !!s->regs[ENET_RDAR];
>>  }
>>
>>  static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 62ad473b05..4bc8f03ec2 100644
>> --- a/include/hw/net/imx_fec.h
>> +++ b/include/hw/net/imx_fec.h
>> @@ -252,6 +252,7 @@ typedef struct IMXFECState {
>>      uint32_t phy_int_mask;
>>
>>      bool is_fec;
>> +    bool needs_flush;
>>  } IMXFECState;
>
> This looks odd -- I don't think you should need extra
> state here. Conceptually what you want is:
>
>  * in the can_receive callback, test some function of
> various bits of device state to decide whether you can
> take data
>  * in the rest of the device, whenever the device state
> changes such that you were previously not able to take
> data but now you can, call qemu_flush_queued_packets().
>
> You shouldn't need any extra state to do this, you just
> need to fix the bug where you have a code path that
> flips ENET_RDAR from 0 to 1 without calling flush
> (you might for instance have a helper function for
> "set ENET_RDAR" that encapsulates setting the state
> and arranging that flush is called).
>

I don't know if you've seen my response to Jason Wang, but I think he
was proposing something similar, and, as I said, that should work fine
and the only reason I didn't do it that way was to avoid doing a flush
every time that host driver drains full RX-ring and gives it back to
the IP block.

I'll give this a try in v2.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 05/17] imx_fec: Use ENET_FTRL to determine truncation length

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 September 2017 at 01:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Andrey,
>>
>> On 09/18/2017 04:50 PM, Andrey Smirnov wrote:
>>>
>>> Frame truncation length, TRUNC_FL, is determined by the contents of
>>> ENET_FTRL register, so convert the code to use it instead of a
>>> hardcoded constant.
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: qemu-devel@nongnu.org
>>> Cc: qemu-arm@nongnu.org
>>> Cc: yurovsky@gmail.com
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>> ---
>>>   hw/net/imx_fec.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>>> index 767402909d..989c11be5f 100644
>>> --- a/hw/net/imx_fec.c
>>> +++ b/hw/net/imx_fec.c
>>> @@ -1050,8 +1050,8 @@ static ssize_t imx_enet_receive(NetClientState *nc,
>>> const uint8_t *buf,
>>>       size += 4;
>>>         /* Huge frames are truncted.  */
>>> -    if (size > ENET_MAX_FRAME_SIZE) {
>>> -        size = ENET_MAX_FRAME_SIZE;
>>> +    if (size > s->regs[ENET_FTRL]) {
>>> +        size = s->regs[ENET_FTRL]; >           flags |= ENET_BD_TR |
>>> ENET_BD_LG;
>>>       }
>>
>>
>> for this to be ok you need to update imx_enet_write(), such:
>>
>>      case ENET_FTRL:
>> -        s->regs[index] = value & 0x00003fff;
>> +        value &= 0x00003fff;
>> +        if (value > ENET_MAX_FRAME_SIZE) {
>> +            warn_report("%s: guest requested bigger "
>> +                        "frame size than QEMU supports "
>> +                        "(%u > %u)", value,
>> +                        ENET_MAX_FRAME_SIZE);
>> +            value = ENET_MAX_FRAME_SIZE;
>> +        }
>> +        s->regs[index] = value;
>>          break;
>
> Yes, and also an incoming-migration post_load callback that
> fails migration if the value is too large.
>
> It might be simpler to truncate to the smaller of the ENET_FTRL
> register value and ENET_MAX_FRAME_SIZE.
>
> (PS: what is the hardware behaviour if ENET_FTRL is set to
> a larger value than ENET_MAX_FRAME_SIZE ?)
>

I haven't tested it in practice, but I assume that hardware should be
capable of receiving packets of up to ENET_RCR[MAX_FL] bytes big (both
RCR[MAX_FL] and FTRL are 14-bits), since that would allow supporting
networks with jumbo frames.

What if ENET_MAX_FRAME_SIZE is increased to 16K instead? Would that
make all of that additional error checking code unnecessary?

Thanks,
Andrey Smirnov

P.S: And sure, I'll fix the typo in v2.

Re: [Qemu-devel] [PATCH 07/17] imx_fec: Emulate SHIFT16 in ENETx_RACC

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:02 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Needed to support latest Linux kernel driver which relies on that
>> functionality.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  hw/net/imx_fec.c         | 23 +++++++++++++++++++++++
>>  include/hw/net/imx_fec.h |  2 ++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index 8a77136d38..bd62d7a75f 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -1037,6 +1037,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>      uint32_t buf_addr;
>>      unsigned int buf_len;
>>      size_t size = len;
>> +    bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
>>
>>      FEC_PRINTF("len %d\n", (int)size);
>>
>> @@ -1049,6 +1050,10 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>      /* 4 bytes for the CRC.  */
>>      size += 4;
>>
>> +    if (shift16) {
>> +        size += 2;
>> +    }
>> +
>>      /* Huge frames are truncted.  */
>>      if (size > s->regs[ENET_FTRL]) {
>>          size = s->regs[ENET_FTRL];
>> @@ -1085,6 +1090,24 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
>>              buf_len += size - 4;
>>          }
>>          buf_addr = bd.data;
>> +
>> +        if (shift16) {
>> +            /*
>> +             * If SHIFT16 bit of ENETx_RACC register is set we need to
>> +             * align the payload to 4-byte boundary.
>> +             */
>> +            const uint8_t zeros[2] = { 0 };
>> +
>> +            dma_memory_write(&address_space_memory, buf_addr,
>> +                             zeros, sizeof(zeros));
>> +
>> +            buf_addr += sizeof(zeros);
>> +            buf_len  -= sizeof(zeros);
>> +
>> +            shift16 = false; /* We only do this once per Ethernet
>> +                              * frame */
>> +        }
>> +
>
> Can you avoid having an end-of-source-line comment that wraps
> to multiple lines, please? (put it on a line of its own, or
> edit down to fit.) Otherwise
>

Sure, will fix in v2.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> More recent version of the IP block support more than one Tx DMA ring,
>> so add the code implementing that feature.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>
> I'm surprised this doesn't mean "we have a property on the device
> to indicate what IP version it is, which the boards set". Or are
> all our current boards mismodelling their ethernet devices with the
> wrong number of TX rings ?
>

As far as I know the only already emulated SoC that this affects is
i.MX6, and, no, it doesn't mismodel its Ethernet device since it has
version of the IP block with only one Tx ring. I didn't add any notion
of versioning because it didn't seem necessary, since 3-ring IP block
should be backwards compatible with 1-ring version and host drivers
written for the latter will end up using only ring #1 of the 3-ring
block.

>> ---
>>  hw/net/imx_fec.c         | 97 +++++++++++++++++++++++++++++++++++++++---------
>>  include/hw/net/imx_fec.h | 23 +++++++++++-
>>  2 files changed, 101 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index bd62d7a75f..6045ffe673 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -196,6 +196,17 @@ static const char *imx_eth_reg_name(IMXFECState *s, uint32_t index)
>>      }
>>  }
>>
>> +static const VMStateDescription vmstate_imx_eth_tx_ring = {
>> +    .name = "fec-tx-ring",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(descriptor, IMXFECTxRing),
>> +        VMSTATE_UINT32(tdsr, IMXFECTxRing),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>>  static const VMStateDescription vmstate_imx_eth = {
>>      .name = TYPE_IMX_FEC,
>>      .version_id = 2,
>> @@ -203,8 +214,10 @@ static const VMStateDescription vmstate_imx_eth = {
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32_ARRAY(regs, IMXFECState, ENET_MAX),
>>          VMSTATE_UINT32(rx_descriptor, IMXFECState),
>> -        VMSTATE_UINT32(tx_descriptor, IMXFECState),
>> -
>> +        VMSTATE_STRUCT_ARRAY(tx_ring, IMXFECState,
>> +                             ENET_TX_RING_NUM,
>> +                             1, vmstate_imx_eth_tx_ring,
>> +                             IMXFECTxRing),
>>          VMSTATE_UINT32(phy_status, IMXFECState),
>>          VMSTATE_UINT32(phy_control, IMXFECState),
>>          VMSTATE_UINT32(phy_advertise, IMXFECState),
>
> This breaks migration compatibility, so you need to figure out
> how you want to handle that. The simple solution is to increment
> the version_id and minimum_version_id fields in vmstate_imx_eth;
> that will mean migration from an old QEMU to a new one doesn't
> work but it fails in a clean way. The complex way is to put the
> new state into a migration subsection with a .needed function
> to determine whether to use it or not. For imx I think we can
> get away with the simple solution.
>

OK, will do in v2.

>> @@ -407,7 +420,7 @@ static void imx_fec_do_tx(IMXFECState *s)
>>      int frame_size = 0, descnt = 0;
>>      uint8_t frame[ENET_MAX_FRAME_SIZE];
>>      uint8_t *ptr = frame;
>> -    uint32_t addr = s->tx_descriptor;
>> +    uint32_t addr = s->tx_ring[0].descriptor;
>>
>>      while (descnt++ < IMX_MAX_DESC) {
>>          IMXFECBufDesc bd;
>> @@ -448,17 +461,38 @@ static void imx_fec_do_tx(IMXFECState *s)
>>          }
>>      }
>>
>> -    s->tx_descriptor = addr;
>> +    s->tx_ring[0].descriptor = addr;
>>
>>      imx_eth_update(s);
>>  }
>>
>> -static void imx_enet_do_tx(IMXFECState *s)
>> +static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
>>  {
>>      int frame_size = 0, descnt = 0;
>>      uint8_t frame[ENET_MAX_FRAME_SIZE];
>>      uint8_t *ptr = frame;
>> -    uint32_t addr = s->tx_descriptor;
>> +    IMXFECTxRing *ring;
>> +    uint32_t addr;
>> +
>> +    switch (index) {
>> +    case ENET_TDAR:
>> +        ring = &s->tx_ring[0];
>> +        break;
>> +    case ENET_TDAR1:
>> +        ring = &s->tx_ring[1];
>> +        break;
>> +    case ENET_TDAR2:
>> +        ring = &s->tx_ring[2];
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: bogus value for index %x\n",
>> +                      __func__, index);
>> +        abort();
>> +        break;
>> +    }
>> +
>> +    addr = ring->descriptor;
>>
>>      while (descnt++ < IMX_MAX_DESC) {
>>          IMXENETBufDesc bd;
>> @@ -502,32 +536,32 @@ static void imx_enet_do_tx(IMXFECState *s)
>>              ptr = frame;
>>              frame_size = 0;
>>              if (bd.option & ENET_BD_TX_INT) {
>> -                s->regs[ENET_EIR] |= ENET_INT_TXF;
>> +                s->regs[ENET_EIR] |= ring->int_txf;
>>              }
>>          }
>>          if (bd.option & ENET_BD_TX_INT) {
>> -            s->regs[ENET_EIR] |= ENET_INT_TXB;
>> +            s->regs[ENET_EIR] |= ring->int_txb;
>>          }
>>          bd.flags &= ~ENET_BD_R;
>>          /* Write back the modified descriptor.  */
>>          imx_enet_write_bd(&bd, addr);
>>          /* Advance to the next descriptor.  */
>>          if ((bd.flags & ENET_BD_W) != 0) {
>> -            addr = s->regs[ENET_TDSR];
>> +            addr = s->regs[ring->tdsr];
>>          } else {
>>              addr += sizeof(bd);
>>          }
>>      }
>>
>> -    s->tx_descriptor = addr;
>> +    ring->descriptor = addr;
>>
>>      imx_eth_update(s);
>>  }
>>
>> -static void imx_eth_do_tx(IMXFECState *s)
>> +static void imx_eth_do_tx(IMXFECState *s, uint32_t index)
>>  {
>>      if (!s->is_fec && (s->regs[ENET_ECR] & ENET_ECR_EN1588)) {
>> -        imx_enet_do_tx(s);
>> +        imx_enet_do_tx(s, index);
>>      } else {
>>          imx_fec_do_tx(s);
>>      }
>> @@ -586,7 +620,22 @@ static void imx_eth_reset(DeviceState *d)
>>      }
>>
>>      s->rx_descriptor = 0;
>> -    s->tx_descriptor = 0;
>> +
>> +    s->tx_ring[0].tdsr = ENET_TDSR;
>> +    s->tx_ring[1].tdsr = ENET_TDSR1;
>> +    s->tx_ring[2].tdsr = ENET_TDSR2;
>> +
>> +    s->tx_ring[0].int_txf = ENET_INT_TXF;
>> +    s->tx_ring[1].int_txf = ENET_INT_TXF1;
>> +    s->tx_ring[2].int_txf = ENET_INT_TXF2;
>> +
>> +    s->tx_ring[0].int_txb = ENET_INT_TXB;
>> +    s->tx_ring[1].int_txb = ENET_INT_TXB1;
>> +    s->tx_ring[2].int_txb = ENET_INT_TXB2;
>> +
>> +    s->tx_ring[0].descriptor = 0;
>> +    s->tx_ring[1].descriptor = 0;
>> +    s->tx_ring[2].descriptor = 0;
>>
>>      /* We also reset the PHY */
>>      phy_reset(s);
>> @@ -814,10 +863,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>>              s->regs[index] = 0;
>>          }
>>          break;
>> +    case ENET_TDAR1:    /* FALLTHROUGH */
>> +    case ENET_TDAR2:    /* FALLTHROUGH */
>>      case ENET_TDAR:
>>          if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>>              s->regs[index] = ENET_TDAR_TDAR;
>> -            imx_eth_do_tx(s);
>> +            imx_eth_do_tx(s, index);
>>          }
>>          s->regs[index] = 0;
>>          break;
>> @@ -829,8 +880,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>>          if ((s->regs[index] & ENET_ECR_ETHEREN) == 0) {
>>              s->regs[ENET_RDAR] = 0;
>>              s->rx_descriptor = s->regs[ENET_RDSR];
>> -            s->regs[ENET_TDAR] = 0;
>> -            s->tx_descriptor = s->regs[ENET_TDSR];
>> +            s->regs[ENET_TDAR]  = 0;
>> +            s->regs[ENET_TDAR1] = 0;
>> +            s->regs[ENET_TDAR2] = 0;
>> +            s->tx_ring[0].descriptor = s->regs[ENET_TDSR];
>> +            s->tx_ring[1].descriptor = s->regs[ENET_TDSR1];
>> +            s->tx_ring[2].descriptor = s->regs[ENET_TDSR2];
>>          }
>>          break;
>>      case ENET_MMFR:
>> @@ -908,7 +963,15 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>>          } else {
>>              s->regs[index] = value & ~7;
>>          }
>> -        s->tx_descriptor = s->regs[index];
>> +        s->tx_ring[0].descriptor = s->regs[index];
>> +        break;
>> +    case ENET_TDSR1:
>> +        s->regs[index] = value & ~7;
>> +        s->tx_ring[1].descriptor = s->regs[index];
>> +        break;
>> +    case ENET_TDSR2:
>> +        s->regs[index] = value & ~7;
>> +        s->tx_ring[2].descriptor = s->regs[index];
>>          break;
>>      case ENET_MRBR:
>>          s->regs[index] = value & 0x00003ff0;
>> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
>> index 20a6aa98b4..40bd29771f 100644
>> --- a/include/hw/net/imx_fec.h
>> +++ b/include/hw/net/imx_fec.h
>> @@ -52,6 +52,8 @@
>>  #define ENET_TFWR              81
>>  #define ENET_FRBR              83
>>  #define ENET_FRSR              84
>> +#define ENET_TDSR1             89
>> +#define ENET_TDSR2             92
>>  #define ENET_RDSR              96
>>  #define ENET_TDSR              97
>>  #define ENET_MRBR              98
>> @@ -66,6 +68,8 @@
>>  #define ENET_FTRL              108
>>  #define ENET_TACC              112
>>  #define ENET_RACC              113
>> +#define ENET_TDAR1             121
>> +#define ENET_TDAR2             123
>>  #define ENET_MIIGSK_CFGR       192
>>  #define ENET_MIIGSK_ENR        194
>>  #define ENET_ATCR              256
>> @@ -106,13 +110,18 @@
>>  #define ENET_INT_WAKEUP        (1 << 17)
>>  #define ENET_INT_TS_AVAIL      (1 << 16)
>>  #define ENET_INT_TS_TIMER      (1 << 15)
>> +#define ENET_INT_TXF2          (1 <<  7)
>> +#define ENET_INT_TXB2          (1 <<  6)
>> +#define ENET_INT_TXF1          (1 <<  3)
>> +#define ENET_INT_TXB1          (1 <<  2)
>>
>>  #define ENET_INT_MAC           (ENET_INT_HB | ENET_INT_BABR | ENET_INT_BABT | \
>>                                  ENET_INT_GRA | ENET_INT_TXF | ENET_INT_TXB | \
>>                                  ENET_INT_RXF | ENET_INT_RXB | ENET_INT_MII | \
>>                                  ENET_INT_EBERR | ENET_INT_LC | ENET_INT_RL | \
>>                                  ENET_INT_UN | ENET_INT_PLR | ENET_INT_WAKEUP | \
>> -                                ENET_INT_TS_AVAIL)
>> +                                ENET_INT_TS_AVAIL | ENET_INT_TXF1 | ENET_INT_TXB1 |\
>> +                                ENET_INT_TXF2 | ENET_INT_TXB2)
>>
>>  /* RDAR */
>>  #define ENET_RDAR_RDAR         (1 << 24)
>> @@ -233,6 +242,15 @@ typedef struct {
>>
>>  #define ENET_BD_BDU            (1 << 31)
>>
>> +#define ENET_TX_RING_NUM       3
>> +
>> +typedef struct IMXFECTxRing {
>> +    uint32_t descriptor;
>> +    uint32_t tdsr;
>> +    uint32_t int_txf;
>> +    uint32_t int_txb;
>
> Why aren't the int_txf and int_txb fields in the migration state?
> What are they for? It looks from the code like they're just constants
> indicating the tx interrupt bits to use for this tx ring, in which
> case they'd be better implemented using a utility function which
> returns the tx bit when given a ring number, I think.
>

I think I just forgot to add those fields to migration data. Any you
are right about their purpose. I'll change the patch to use functions
in v2.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 13/17] i.MX: Add code to emulate i.MX2 watchdog IP block

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Add enough code to emulate i.MX2 watchdog IP block so it would be
>> possible to reboot the machine running Linux Guest.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  hw/misc/Makefile.objs      |   1 +
>>  hw/misc/imx2_wdt.c         | 117 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/misc/imx2_wdt.h |  36 ++++++++++++++
>>  3 files changed, 154 insertions(+)
>>  create mode 100644 hw/misc/imx2_wdt.c
>>  create mode 100644 include/hw/misc/imx2_wdt.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index ac1be05a03..c393a93456 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_IMX) += imx25_ccm.o
>>  obj-$(CONFIG_IMX) += imx6_ccm.o
>>  obj-$(CONFIG_IMX) += imx6_src.o
>>  obj-$(CONFIG_IMX) += imx7_ccm.o
>> +obj-$(CONFIG_IMX) += imx2_wdt.o
>>  obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>>  obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>>  obj-$(CONFIG_MAINSTONE) += mst_fpga.o
>> diff --git a/hw/misc/imx2_wdt.c b/hw/misc/imx2_wdt.c
>> new file mode 100644
>> index 0000000000..9d97a19511
>> --- /dev/null
>> +++ b/hw/misc/imx2_wdt.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * i.MX2 Watchdog IP block
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * 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 "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/hw.h"
>> +#include "exec/memory.h"
>> +#include "exec/address-spaces.h"
>> +#include "hw/sysbus.h"
>> +#include "qemu/log.h"
>> +#include "qemu/timer.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/watchdog.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/sizes.h"
>
> That's an awful lot of includes for a very simple device. Are
> you sure they're all needed?

No, I am not sure. I'll double-check.

>
>> +
>> +#include "hw/misc/imx2_wdt.h"
>> +
>> +
>> +#define IMX2_WDT_WCR_WDA       BIT(5)          /* -> External Reset WDOG_B */
>> +#define IMX2_WDT_WCR_SRS       BIT(4)          /* -> Software Reset Signal */
>> +
>> +static uint64_t imx2_wdt_read(void *opaque, hwaddr addr,
>> +                              unsigned int size)
>> +{
>> +    IMX2WdtState *s = opaque;
>> +    const size_t index = addr / sizeof(s->reg[0]);
>> +
>> +    if (index < ARRAY_SIZE(s->reg))
>> +        return s->reg[index];
>> +    else
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
>> +
>> +    return 0xDEADBEEF;
>> +}
>> +
>> +static void imx2_wdt_write(void *opaque, hwaddr addr,
>> +                           uint64_t val64, unsigned int size)
>> +{
>> +    uint16_t value  = val64;
>> +    IMX2WdtState *s = opaque;
>> +    const size_t index = addr / sizeof(s->reg[0]);
>> +
>> +    switch (index) {
>> +    case IMX2_WDT_WCR:
>> +        if (value & (IMX2_WDT_WCR_WDA | IMX2_WDT_WCR_SRS))
>> +            watchdog_perform_action();
>
> Missing "break"?
>

Oops, good catch! Will fix in v2.

> Also checkpatch should tell you you need more braces.
>

Yeah, I definitely forgot to run this through checkpatch, sorry.

>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps imx2_wdt_ops = {
>> +    .read  = imx2_wdt_read,
>> +    .write = imx2_wdt_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.
>> +         */
>
> This sounds like it's perhaps a job for .impl.unaligned
> rather than .valid.unaligned ?
>

This was a snippet I copied from some already existing emulation
block, so I didn't pay too much attention. But yeah, now that you
suggested it, I think I should've used impl.valid as you suggest. Will
fix in v2.

>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +        .unaligned = false,
>> +    },
>> +};
>> +
>> +static void imx2_wdt_realize(DeviceState *dev, Error **errp)
>> +{
>> +    IMX2WdtState *s = IMX2_WDT(dev);
>> +
>> +    memory_region_init_io(&s->mmio, OBJECT(dev),
>> +                          &imx2_wdt_ops, s,
>> +                          TYPE_IMX2_WDT".mmio", SZ_64K);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>> +}
>> +
>> +static void imx2_wdt_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = imx2_wdt_realize;
>> +
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static const TypeInfo imx2_wdt_info = {
>> +    .name          = TYPE_IMX2_WDT,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(IMX2WdtState),
>> +    .class_init    = imx2_wdt_class_init,
>> +};
>
> This device needs a reset function and vmstate description.
>

OK, will fix in v2.

Thanks
Andrey Smirnov

Re: [Qemu-devel] [PATCH 15/17] include/qemu: Add sizes.h from Linux

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:13 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Add sizes.h from Linux to have a more readable way of specifying
>> MemoryRegion sizes.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  include/qemu/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 include/qemu/sizes.h
>>
>> diff --git a/include/qemu/sizes.h b/include/qemu/sizes.h
>> new file mode 100644
>> index 0000000000..9aedb9f8f6
>> --- /dev/null
>> +++ b/include/qemu/sizes.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * Copy of include/linux/sizes.h
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>
> Not really convinced of the utility, and it's gpl-2-only, so no thanks.
>

OK, will drop in v2.

Thanks,
Andrey Smironv

Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:38 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> For now we only support the following devices:
>>     * up to 2 Cortex A9 cores (SMP works with PSCI)
>>     * A7 MPCORE (identical to A15 MPCORE)
>>     * 7 i.MX UARTs
>>     * 1 CCM device
>>     * 2 Ethernet controllers (FEC)
>>     * 3 SD controllers (USDHC)
>>     * 1 SNVS device
>>     * 1 WDT device
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  default-configs/arm-softmmu.mak |   1 +
>>  hw/arm/Makefile.objs            |   2 +
>>  hw/arm/fsl-imx7.c               | 327 ++++++++++++++++++++++++++++++++++++++++
>>  include/hw/arm/fsl-imx7.h       | 114 ++++++++++++++
>>  4 files changed, 444 insertions(+)
>>  create mode 100644 hw/arm/fsl-imx7.c
>>  create mode 100644 include/hw/arm/fsl-imx7.h
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index bbdd3c1d8b..98396a3ad2 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
>>  CONFIG_FSL_IMX6=y
>>  CONFIG_FSL_IMX31=y
>>  CONFIG_FSL_IMX25=y
>> +CONFIG_FSL_IMX7=y
>>
>>  CONFIG_IMX_I2C=y
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index a2e56ecaae..33f6051ae3 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>>  obj-$(CONFIG_MPS2) += mps2.o
>> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
>> +
>> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
>> new file mode 100644
>> index 0000000000..bd01bb7f59
>> --- /dev/null
>> +++ b/hw/arm/fsl-imx7.c
>> @@ -0,0 +1,327 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * i.MX7 SoC definitions
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * Based on hw/arm/fsl-imx6.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>
> We pretty strongly prefer GPL-2-or-later for new code, not 2-only.
>

OK, will change in v2.

>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/arm/fsl-imx7.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qemu/error-report.h"
>> +
>> +#define NAME_SIZE 20
>> +
>> +#define for_each_cpu(i)     for (i = 0; i < smp_cpus; i++)
>
> Just open-code this, please.

Sure, will do in v2.

>
>> +
>> +static void fsl_imx7_init(Object *obj)
>> +{
>> +    BusState *sysbus = sysbus_get_default();
>> +    FslIMX7State *s = FSL_IMX7(obj);
>> +    char name[NAME_SIZE];
>> +    int i;
>> +
>> +    if (smp_cpus > FSL_IMX7_NUM_CPUS) {
>> +        error_report("%s: Only %d CPUs are supported (%d requested)",
>> +                     TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
>> +        exit(1);
>> +    }
>> +
>> +    for_each_cpu(i) {
>> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
>> +                          "cortex-a7-" TYPE_ARM_CPU);
>> +        snprintf(name, NAME_SIZE, "cpu%d", i);
>> +        object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
>> +                                  &error_fatal);
>> +    }
>> +
>> +    /*
>> +     * A7MPCORE
>> +     */
>> +    object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);
>
> Stealing the A15MPCORE device for a7 is kind of ugly, but I can't
> decide what would be better instead...
>

I agree, but I looked through the RMs for both and didn't find any
software-visible differences that would warrant creating a standalone
A7MPCORE type.

>> +    qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
>> +    object_property_add_child(obj, "a7mpcore",
>> +                              OBJECT(&s->a7mpcore), &error_fatal);
>> +
>> +    /*
>> +     * CCM
>> +     */
>> +    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
>> +    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
>> +    object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
>> +
>> +    /*
>> +     * UART
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
>> +            object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
>> +            qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
>> +            snprintf(name, NAME_SIZE, "uart%d", i);
>> +            object_property_add_child(obj, name, OBJECT(&s->uart[i]),
>> +                                      &error_fatal);
>> +    }
>> +
>> +    /*
>> +     * Ethernet
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
>> +            object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
>> +            qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
>> +            snprintf(name, NAME_SIZE, "eth%d", i);
>> +            object_property_add_child(obj, name, OBJECT(&s->eth[i]),
>> +                                      &error_fatal);
>> +    }
>> +
>> +    /*
>> +     * SDHCI
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> +            object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
>> +                              TYPE_IMX_USDHC);
>> +            qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
>> +            snprintf(name, NAME_SIZE, "usdhc%d", i);
>> +            object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
>> +                                      &error_fatal);
>> +    }
>> +
>> +    /*
>> +     * SNVS
>> +     */
>> +    object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
>> +    qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
>> +    object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
>> +
>> +    /*
>> +     * Watchdog
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
>> +            object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
>> +            qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
>> +            snprintf(name, NAME_SIZE, "wdt%d", i);
>> +            object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
>> +                                      &error_fatal);
>> +    }
>> +}
>> +
>> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>> +{
>> +    FslIMX7State *s = FSL_IMX7(dev);
>> +    Object *o;
>> +    int i;
>> +    qemu_irq irq;
>> +
>> +    for_each_cpu(i) {
>> +       o = OBJECT(&s->cpu[i]);
>> +
>> +        object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
>> +                                "psci-conduit", &error_abort);
>> +
>> +        object_property_set_bool(o, false, "has_el3", &error_abort);
>
> Does this SoC's CPU really not have EL3?
>

The SoC support TrustZone, so I thing it does have EL3. This setting
however was breaking SMP support(I can't remember more details) and
that's why I added the code setting it to "false".


>> +
>> +        /* On uniprocessor, the CBAR is set to 0 */
>> +        if (smp_cpus > 1) {
>> +            object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
>> +                                    "reset-cbar", &error_abort);
>> +        }
>> +
>> +        if (i) {
>> +            /* Secondary CPUs start in PSCI powered-down state */
>> +            object_property_set_bool(o, true,
>> +                                     "start-powered-off", &error_abort);
>> +        }
>> +
>> +        object_property_set_bool(o, true, "realized", &error_abort);
>> +    }
>> +
>> +    /*
>> +     * A7MPCORE
>> +     */
>> +    object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
>> +                            &error_abort);
>> +    object_property_set_int(OBJECT(&s->a7mpcore),
>> +                            FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
>> +                            "num-irq", &error_abort);
>> +
>> +    object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
>> +                             &error_abort);
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
>> +
>> +    for_each_cpu(i) {
>> +        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
>> +        DeviceState  *d   = DEVICE(qemu_get_cpu(i));
>> +
>> +        irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
>> +        sysbus_connect_irq(sbd, i, irq);
>> +        irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
>> +        sysbus_connect_irq(sbd, i + smp_cpus, irq);
>> +    }
>> +
>> +    /*
>> +     * CCM
>> +     */
>> +    object_property_set_bool(OBJECT(&s->ccm), true, "realized", &error_abort);
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
>> +
>> +    /*
>> +     * UART
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
>> +        static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
>> +            FSL_IMX7_UART1_ADDR,
>> +            FSL_IMX7_UART2_ADDR,
>> +            FSL_IMX7_UART3_ADDR,
>> +            FSL_IMX7_UART4_ADDR,
>> +            FSL_IMX7_UART5_ADDR,
>> +            FSL_IMX7_UART6_ADDR,
>> +            FSL_IMX7_UART7_ADDR,
>> +        };
>> +
>> +        static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
>> +            FSL_IMX7_UART1_IRQ,
>> +            FSL_IMX7_UART2_IRQ,
>> +            FSL_IMX7_UART3_IRQ,
>> +            FSL_IMX7_UART4_IRQ,
>> +            FSL_IMX7_UART5_IRQ,
>> +            FSL_IMX7_UART6_IRQ,
>> +            FSL_IMX7_UART7_IRQ,
>> +        };
>> +
>> +
>> +        if (i < MAX_SERIAL_PORTS) {
>> +            Chardev *chr;
>> +            chr = serial_hds[i];
>> +
>> +            if (!chr) {
>> +                char *label = g_strdup_printf("imx7.uart%d", i + 1);
>> +                chr = qemu_chr_new(label, "null");
>> +                g_free(label);
>> +                serial_hds[i] = chr;
>> +            }
>
> I think the conclusion we've come to about chardevs is that the
> UART should handle being passed a NULL chardev, rather than the
> board code having to create a dummy chardev to pass to it.
>

OK. Will fix in v2.

>> +
>> +            qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
>> +        }
>> +
>> +        object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
>> +                                 &error_abort);
>> +
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, FSL_IMX7_UARTn_ADDR[i]);
>> +
>> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
>> +    }
>> +
>> +    /*
>> +     * Etherenet
>
> Typo: "Ethernet"
>

Will fix in v2.

>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
>> +        static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
>> +            FSL_IMX7_ENET1_ADDR,
>> +            FSL_IMX7_ENET2_ADDR,
>> +        };
>> +
>> +        qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
>> +        object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
>> +                                 &error_abort);
>> +
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
>> +
>> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 0));
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
>> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 3));
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
>> +    }
>> +
>> +    /*
>> +     * USDHC
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> +        static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
>> +            FSL_IMX7_USDHC1_ADDR,
>> +            FSL_IMX7_USDHC2_ADDR,
>> +            FSL_IMX7_USDHC3_ADDR,
>> +        };
>> +
>> +        static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
>> +            FSL_IMX7_USDHC1_IRQ,
>> +            FSL_IMX7_USDHC2_IRQ,
>> +            FSL_IMX7_USDHC3_IRQ,
>> +        };
>> +
>> +        object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
>> +                                 &error_abort);
>> +
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
>> +                        FSL_IMX7_USDHCn_ADDR[i]);
>> +
>> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
>> +    }
>> +
>> +    /*
>> +     * SNVS
>> +     */
>> +    object_property_set_bool(OBJECT(&s->snvs), true, "realized", &error_abort);
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
>> +
>> +    /*
>> +     * Watchdog
>> +     */
>> +    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
>> +        static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
>> +            FSL_IMX7_WDOG1_ADDR,
>> +            FSL_IMX7_WDOG2_ADDR,
>> +            FSL_IMX7_WDOG3_ADDR,
>> +            FSL_IMX7_WDOG4_ADDR,
>> +        };
>> +
>> +        object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
>> +                                 &error_abort);
>> +
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, FSL_IMX7_WDOGn_ADDR[i]);
>> +    }
>> +}
>> +
>> +static void fsl_imx7_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->realize = fsl_imx7_realize;
>> +
>> +    /*
>> +     * Reason: creates an ARM CPU, thus use after free(), see
>> +     * arm_cpu_class_init()
>> +     */
>
> I think this is the wrong reason (we've fixed that problem, IIRC).
> You do want this not to be user-creatable, but the reason is
> /* Reason: uses serial_hds and nd_table in realize() */
> (compare the reason text in the aspeed/allwinner/digic/xlnx-zynqmp
> SoC device objects).
>

That's another copy-pasted snippet, so I am not surprised to learn it
is wrong. I'll take a look at Zynq emulation and update/fix this code
in v2.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 20:51, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.org
>> Cc: yurovsky@gmail.com
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  hw/arm/Makefile.objs   |   2 +-
>>  hw/arm/mcimx7d-sabre.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 101 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/arm/mcimx7d-sabre.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 33f6051ae3..fc4a963de8 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>>  obj-$(CONFIG_MPS2) += mps2.o
>> -obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
>> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
>>
>> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
>> new file mode 100644
>> index 0000000000..34e3933db8
>> --- /dev/null
>> +++ b/hw/arm/mcimx7d-sabre.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * MCIMX7D_SABRE Board System emulation.
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * This code is licensed under the GPL, version 2 or later.
>> + * See the file `COPYING' in the top level directory.
>> + *
>> + * It (partially) emulates a mcimx7d_sabre board, with a Freescale
>> + * i.MX7 SoC
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/arm/fsl-imx7.h"
>> +#include "hw/boards.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/device_tree.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/qtest.h"
>> +#include "net/net.h"
>> +
>> +typedef struct {
>> +    FslIMX7State soc;
>> +    MemoryRegion ram;
>> +} MCIMX7Sabre;
>> +
>> +static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, void *fdt)
>> +{
>> +    const char comp[] = "arm,psci-0.2\0arm,psci";
>> +
>> +    qemu_fdt_add_subnode(fdt, "/psci");
>> +    qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
>> +    qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
>> +}
>
> You shouldn't need this -- we should just be able to work with whatever
> the real hardware's device tree is. ("virt" is a special case because
> we create our own device tree there.)
>

Upstream kernel for i.MX7 relies on PSCI to support SMP and this is a
kind of DT fixup that would normally be done by a PSCI-compatible
bootloader. I can remove this code, but it would effectively disable
SMP on vanilla DT/kernel combos.

>> +
>> +static void mcimx7d_sabre_init(MachineState *machine)
>> +{
>> +    static struct arm_boot_info boot_info;
>> +    MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
>> +    Object *soc;
>> +    int i;
>> +
>> +    if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
>> +        error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
>> +                     machine->ram_size, FSL_IMX7_MMDC_SIZE);
>> +        exit(1);
>> +    }
>> +
>> +    boot_info = (struct arm_boot_info) {
>> +        .loader_start = FSL_IMX7_MMDC_ADDR,
>> +        .board_id = -1,
>> +        .ram_size = machine->ram_size,
>> +        .kernel_filename = machine->kernel_filename,
>> +        .kernel_cmdline = machine->kernel_cmdline,
>> +        .initrd_filename = machine->initrd_filename,
>> +        .nb_cpus = smp_cpus,
>> +        .modify_dtb = mcimx7d_add_psci_node,
>> +    };
>> +
>> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
>> +    soc = OBJECT(&s->soc);
>> +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
>> +    object_property_set_bool(soc, true, "realized", &error_fatal);
>> +
>> +    memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
>> +                                         machine->ram_size);
>> +    memory_region_add_subregion(get_system_memory(),
>> +                                FSL_IMX7_MMDC_ADDR, &s->ram);
>> +
>> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> +        BusState *bus;
>> +        DeviceState *carddev;
>> +        DriveInfo *di;
>> +        BlockBackend *blk;
>> +
>> +        di = drive_get_next(IF_SD);
>> +        blk = di ? blk_by_legacy_dinfo(di) : NULL;
>> +        bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
>> +        carddev = qdev_create(bus, TYPE_SD_CARD);
>> +        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
>> +        object_property_set_bool(OBJECT(carddev), true, "realized", &error_fatal);
>> +    }
>> +
>> +    if (!qtest_enabled()) {
>> +        arm_load_kernel(&s->soc.cpu[0], &boot_info);
>> +    }
>> +}
>> +
>> +static void mcimx7d_sabre_machine_init(MachineClass *mc)
>> +{
>> +    mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
>> +    mc->init = mcimx7d_sabre_init;
>> +    mc->max_cpus = FSL_IMX7_NUM_CPUS;
>> +    mc->ignore_memory_transaction_failures = true;
>
> Please don't set this flag on new board models -- it is only
> for legacy existing board models (where we don't know what
> running guest code might be broken by turning bad accesses
> into guest aborts. For a new board model, you should leave
> the flag unset and instead stub out any devices that you
> need to using create_unimplemented_device() so that your
> guest code boots.

That flag didn't exist when I started porting this board and I only
discovered it a couple of hours before I prepared v1 of the patchset,
since I didn't want to submit the code that wouldn't work but didn't
have time to implements all of the missing blocks. I'll update v2, to
get rid of it.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH 02/17] imx_fec: Do not calculate FEC

[Peter Maydell]

On 9 October 2017 at 15:47, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 6:48 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This looks a bit odd. Doesn't the hardware calculate the CRC here?
>>
>
> It does, it just seemed to me that since the hardware also has a "CRC
> error" bit in its status register, there would be few if any users of
> the actual calculated CRC value. Given how eTSEC emulation layer gets
> away without calculating CRC I thought that it might be possible to
> have this optimization here as well.

As a general rule QEMU should just emulate what the hardware does
(even if the usual guest software doesn't happen to care about
that corner of the specification).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/17] imx_fec: Add support for multiple Tx DMA rings

[Peter Maydell]

On 9 October 2017 at 16:38, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 7:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>>> More recent version of the IP block support more than one Tx DMA ring,
>>> so add the code implementing that feature.
>>>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: qemu-devel@nongnu.org
>>> Cc: qemu-arm@nongnu.org
>>> Cc: yurovsky@gmail.com
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>
>> I'm surprised this doesn't mean "we have a property on the device
>> to indicate what IP version it is, which the boards set". Or are
>> all our current boards mismodelling their ethernet devices with the
>> wrong number of TX rings ?
>>
>
> As far as I know the only already emulated SoC that this affects is
> i.MX6, and, no, it doesn't mismodel its Ethernet device since it has
> version of the IP block with only one Tx ring. I didn't add any notion
> of versioning because it didn't seem necessary, since 3-ring IP block
> should be backwards compatible with 1-ring version and host drivers
> written for the latter will end up using only ring #1 of the 3-ring
> block.

It is guest visible if the guest looks for it, though, so I think
we should have a device property to set the number of Tx rings
(or to set the version number of the IP if that's in a guest
visible register and a more useful way to model it) so we can
get it right for both boards.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.

[Peter Maydell]

On 9 October 2017 at 17:18, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 6, 2017 at 7:38 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 September 2017 at 20:50, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>>> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    FslIMX7State *s = FSL_IMX7(dev);
>>> +    Object *o;
>>> +    int i;
>>> +    qemu_irq irq;
>>> +
>>> +    for_each_cpu(i) {
>>> +       o = OBJECT(&s->cpu[i]);
>>> +
>>> +        object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
>>> +                                "psci-conduit", &error_abort);
>>> +
>>> +        object_property_set_bool(o, false, "has_el3", &error_abort);
>>
>> Does this SoC's CPU really not have EL3?
>>
>
> The SoC support TrustZone, so I thing it does have EL3. This setting
> however was breaking SMP support(I can't remember more details) and
> that's why I added the code setting it to "false".

The idea is that for new boards (where we start from "no guest
code works") we try to model the hardware correctly from the
start, ie figure out why SMP doesn't work if you say you have
EL3 (probably because either you need to provide boot code
for the other CPUs or implement a power controller, depending
on how SMP secondary CPU start works on this hardware).
Existing board models that were added before we got EL3 working
tend to set has_el3 to false even though the h/w we're modelling
has EL3, because they're maintaining backwards compatibility with
previous released versions of QEMU where we didn't implement EL3.
New boards don't have that back-compat requirement, so if it's
not too hard to get it working from the start that's preferable.

thanks
-- PMM