[Qemu-devel] [PATCH v2 0/5] Improve the Cadence GEM multi-queue support

[Qemu-devel] [PATCH v2 0/5] Improve the Cadence GEM multi-queue support

[Alistair Francis]

Improve the Cadence GEM multi-queue support. This fixes a few bugs
which were hanging around from the initial implementation.

V2:
 - Fix up the interrupt updating logic and consolidate all the updates
   to a single function.
 - Improve the debug print information

Alistair Francis (5):
  cadence_gem: Read the correct queue descriptor
  cadence_gem: Correct the multi-queue can rx logic
  cadence_gem: Correct the interupt logic
  cadence_gem: Make the revision a property
  xlnx-zynqmp: Set the Cadence GEM revision

 hw/arm/xlnx-zynqmp.c         |  6 +++++-
 hw/net/cadence_gem.c         | 45 +++++++++++++++++++++++++++++---------------
 include/hw/net/cadence_gem.h |  1 +
 3 files changed, 36 insertions(+), 16 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/5] cadence_gem: Read the correct queue descriptor

[Alistair Francis]

Read the correct descriptor instead of hardcoding the first (q=0).

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 hw/net/cadence_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index d4de8ad..17c229d 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -790,8 +790,8 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
 {
     DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
     /* read current descriptor */
-    cpu_physical_memory_read(s->rx_desc_addr[0],
-                             (uint8_t *)s->rx_desc[0], sizeof(s->rx_desc[0]));
+    cpu_physical_memory_read(s->rx_desc_addr[q],
+                             (uint8_t *)s->rx_desc[q], sizeof(s->rx_desc[q]));
 
     /* Descriptor owned by software ? */
     if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/5] cadence_gem: Correct the multi-queue can rx logic

[Alistair Francis]

Correct the buffer descriptor busy logic to work correctly when using
multiple queues.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/net/cadence_gem.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 17c229d..a66a9cc 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -481,14 +481,17 @@ static int gem_can_receive(NetClientState *nc)
     }
 
     for (i = 0; i < s->num_priority_queues; i++) {
-        if (rx_desc_get_ownership(s->rx_desc[i]) == 1) {
-            if (s->can_rx_state != 2) {
-                s->can_rx_state = 2;
-                DB_PRINT("can't receive - busy buffer descriptor (q%d) 0x%x\n",
-                         i, s->rx_desc_addr[i]);
-             }
-            return 0;
+        if (rx_desc_get_ownership(s->rx_desc[i]) != 1) {
+            break;
+        }
+    };
+
+    if (i == s->num_priority_queues) {
+        if (s->can_rx_state != 2) {
+            s->can_rx_state = 2;
+            DB_PRINT("can't receive - all the buffer descriptors are busy\n");
         }
+        return 0;
     }
 
     if (s->can_rx_state != 0) {
-- 
2.9.3

[Qemu-devel] [PATCH v2 3/5] cadence_gem: Correct the interupt logic

[Alistair Francis]

This patch fixes two mistakes in the interrupt logic.

First we only trigger single-queue or multi-queue interrupts if the status
register is set. This logic was already used for non multi-queue interrupts
but it also applies to multi-queue interrupts.

Secondly we need to lower the interrupts if the ISR isn't set. As part
of this we can remove the other interrupt lowering logic and consolidate
it inside gem_update_int_status().

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/net/cadence_gem.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index a66a9cc..fc3a184 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -509,7 +509,18 @@ static void gem_update_int_status(CadenceGEMState *s)
 {
     int i;
 
-    if ((s->num_priority_queues == 1) && s->regs[GEM_ISR]) {
+    if (!s->regs[GEM_ISR]) {
+        /* ISR isn't set, clear all the interrupts */
+        for (i = 0; i < s->num_priority_queues; ++i) {
+            qemu_set_irq(s->irq[i], 0);
+        }
+        return;
+    }
+
+    /* If we get here we know s->regs[GEM_ISR] is set, so we don't need to
+     * check it again.
+     */
+    if (s->num_priority_queues == 1) {
         /* No priority queues, just trigger the interrupt */
         DB_PRINT("asserting int.\n");
         qemu_set_irq(s->irq[0], 1);
@@ -1274,7 +1285,6 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
 {
     CadenceGEMState *s;
     uint32_t retval;
-    int i;
     s = (CadenceGEMState *)opaque;
 
     offset >>= 2;
@@ -1285,9 +1295,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
     switch (offset) {
     case GEM_ISR:
         DB_PRINT("lowering irqs on ISR read\n");
-        for (i = 0; i < s->num_priority_queues; ++i) {
-            qemu_set_irq(s->irq[i], 0);
-        }
+        gem_update_int_status(s);
         break;
     case GEM_PHYMNTNC:
         if (retval & GEM_PHYMNTNC_OP_R) {
-- 
2.9.3

[Qemu-devel] [PATCH v2 4/5] cadence_gem: Make the revision a property

[Alistair Francis]

Expose the Cadence GEM revision as a property.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 hw/net/cadence_gem.c         | 6 +++++-
 include/hw/net/cadence_gem.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index fc3a184..fe0a49f 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -300,6 +300,8 @@
 #define DESC_1_RX_SOF 0x00004000
 #define DESC_1_RX_EOF 0x00008000
 
+#define GEM_MODID_VALUE 0x00020118
+
 static inline unsigned tx_desc_get_buffer(unsigned *desc)
 {
     return desc[0];
@@ -1223,7 +1225,7 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_TXPAUSE] = 0x0000ffff;
     s->regs[GEM_TXPARTIALSF] = 0x000003ff;
     s->regs[GEM_RXPARTIALSF] = 0x000003ff;
-    s->regs[GEM_MODID] = 0x00020118;
+    s->regs[GEM_MODID] = s->revision;
     s->regs[GEM_DESCONF] = 0x02500111;
     s->regs[GEM_DESCONF2] = 0x2ab13fff;
     s->regs[GEM_DESCONF5] = 0x002f2145;
@@ -1519,6 +1521,8 @@ static const VMStateDescription vmstate_cadence_gem = {
 
 static Property gem_properties[] = {
     DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
+    DEFINE_PROP_UINT32("revision", CadenceGEMState, revision,
+                       GEM_MODID_VALUE),
     DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
                       num_priority_queues, 1),
     DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
index c469ffe..35de622 100644
--- a/include/hw/net/cadence_gem.h
+++ b/include/hw/net/cadence_gem.h
@@ -50,6 +50,7 @@ typedef struct CadenceGEMState {
     uint8_t num_priority_queues;
     uint8_t num_type1_screeners;
     uint8_t num_type2_screeners;
+    uint32_t revision;
 
     /* GEM registers backing store */
     uint32_t regs[CADENCE_GEM_MAXREG];
-- 
2.9.3

[Qemu-devel] [PATCH v2 5/5] xlnx-zynqmp: Set the Cadence GEM revision

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 hw/arm/xlnx-zynqmp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index bc4e66b..e41b6fe 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -30,6 +30,8 @@
 #define ARM_PHYS_TIMER_PPI  30
 #define ARM_VIRT_TIMER_PPI  27
 
+#define GEM_REVISION        0x40070106
+
 #define GIC_BASE_ADDR       0xf9000000
 #define GIC_DIST_ADDR       0xf9010000
 #define GIC_CPU_ADDR        0xf9020000
@@ -334,8 +336,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
             qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
             qdev_set_nic_properties(DEVICE(&s->gem[i]), nd);
         }
+        object_property_set_int(OBJECT(&s->gem[i]), GEM_REVISION, "revision",
+                                &error_abort);
         object_property_set_int(OBJECT(&s->gem[i]), 2, "num-priority-queues",
-                                  &error_abort);
+                                &error_abort);
         object_property_set_bool(OBJECT(&s->gem[i]), true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 3/5] cadence_gem: Correct the interupt logic

[Alistair Francis]

On Mon, Apr 10, 2017 at 4:43 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch fixes two mistakes in the interrupt logic.
>
> First we only trigger single-queue or multi-queue interrupts if the status
> register is set. This logic was already used for non multi-queue interrupts
> but it also applies to multi-queue interrupts.
>
> Secondly we need to lower the interrupts if the ISR isn't set. As part
> of this we can remove the other interrupt lowering logic and consolidate
> it inside gem_update_int_status().
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

This patch can be cleaned up more, I'll send a V3.

Thanks,

Alistair

> ---
>
>  hw/net/cadence_gem.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index a66a9cc..fc3a184 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -509,7 +509,18 @@ static void gem_update_int_status(CadenceGEMState *s)
>  {
>      int i;
>
> -    if ((s->num_priority_queues == 1) && s->regs[GEM_ISR]) {
> +    if (!s->regs[GEM_ISR]) {
> +        /* ISR isn't set, clear all the interrupts */
> +        for (i = 0; i < s->num_priority_queues; ++i) {
> +            qemu_set_irq(s->irq[i], 0);
> +        }
> +        return;
> +    }
> +
> +    /* If we get here we know s->regs[GEM_ISR] is set, so we don't need to
> +     * check it again.
> +     */
> +    if (s->num_priority_queues == 1) {
>          /* No priority queues, just trigger the interrupt */
>          DB_PRINT("asserting int.\n");
>          qemu_set_irq(s->irq[0], 1);
> @@ -1274,7 +1285,6 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      CadenceGEMState *s;
>      uint32_t retval;
> -    int i;
>      s = (CadenceGEMState *)opaque;
>
>      offset >>= 2;
> @@ -1285,9 +1295,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>      switch (offset) {
>      case GEM_ISR:
>          DB_PRINT("lowering irqs on ISR read\n");
> -        for (i = 0; i < s->num_priority_queues; ++i) {
> -            qemu_set_irq(s->irq[i], 0);
> -        }
> +        gem_update_int_status(s);
>          break;
>      case GEM_PHYMNTNC:
>          if (retval & GEM_PHYMNTNC_OP_R) {
> --
> 2.9.3
>