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

[Qemu-devel] [PATCH v1 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.

Alistair Francis (5):
  cadence_gem: Read the correct queue descriptor
  cadence_gem: Correct the multi-queue can rx logic
  cadence_gem: Only trigger interrupts if the status register is set
  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         | 30 +++++++++++++++++++-----------
 include/hw/net/cadence_gem.h |  1 +
 3 files changed, 25 insertions(+), 12 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v1 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>
---

 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 v1 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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 17c229d..3e37665 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -481,14 +481,18 @@ 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 - busy buffer descriptor (q%d) 0x%x\n",
+                     i, s->rx_desc_addr[i]);
         }
+        return 0;
     }
 
     if (s->can_rx_state != 0) {
-- 
2.9.3

[Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Alistair Francis]

Only trigger multi-queue GEM interrupts if the interrupt status register
is set. This logic was already used for non multi-queue interrupts but
it also applies to multi-queue interrupts.

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

 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3e37665..b9eaed4 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
     }
 
     for (i = 0; i < s->num_priority_queues; ++i) {
-        if (s->regs[GEM_INT_Q1_STATUS + i]) {
+        if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
             DB_PRINT("asserting int. (q=%d)\n", i);
             qemu_set_irq(s->irq[i], 1);
         }
-- 
2.9.3

[Qemu-devel] [PATCH v1 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>
---

 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 b9eaed4..7047e02 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];
@@ -1213,7 +1215,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;
@@ -1512,6 +1514,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 v1 5/5] xlnx-zynqmp: Set the Cadence GEM revision

[Alistair Francis]

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

 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 v1 1/5] cadence_gem: Read the correct queue descriptor

[Philippe Mathieu-Daudé]

On 04/04/2017 08:40 PM, Alistair Francis wrote:
> 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>

> ---
>
>  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) {
>

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

[Philippe Mathieu-Daudé]

On 04/04/2017 08:40 PM, Alistair Francis wrote:
> 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>

> ---
>
>  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 b9eaed4..7047e02 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];
> @@ -1213,7 +1215,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;
> @@ -1512,6 +1514,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];
>

Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Philippe Mathieu-Daudé]

On 04/04/2017 08:40 PM, Alistair Francis wrote:
> Only trigger multi-queue GEM interrupts if the interrupt status register
> is set. This logic was already used for non multi-queue interrupts but
> it also applies to multi-queue interrupts.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

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

> ---
>
>  hw/net/cadence_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3e37665..b9eaed4 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
>      }
>
>      for (i = 0; i < s->num_priority_queues; ++i) {
> -        if (s->regs[GEM_INT_Q1_STATUS + i]) {
> +        if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
>              DB_PRINT("asserting int. (q=%d)\n", i);
>              qemu_set_irq(s->irq[i], 1);
>          }
>

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

[Peter Maydell]

On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 17c229d..3e37665 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -481,14 +481,18 @@ 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 - busy buffer descriptor (q%d) 0x%x\n",
> +                     i, s->rx_desc_addr[i]);

This looks a little odd -- surely i isn't the right index to use
into rx_desc_addr[] any more now we're outside the loop and i
is always larger than the largest valid queue number? It looks
like the debug print should be rephrased somehow.

thanks
-- PMM

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

[Peter Maydell]

On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  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

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

thanks
-- PMM

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

[Peter Maydell]

On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Read the correct descriptor instead of hardcoding the first (q=0).
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  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
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Peter Maydell]

On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Only trigger multi-queue GEM interrupts if the interrupt status register
> is set. This logic was already used for non multi-queue interrupts but
> it also applies to multi-queue interrupts.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  hw/net/cadence_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3e37665..b9eaed4 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
>      }
>
>      for (i = 0; i < s->num_priority_queues; ++i) {
> -        if (s->regs[GEM_INT_Q1_STATUS + i]) {
> +        if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
>              DB_PRINT("asserting int. (q=%d)\n", i);
>              qemu_set_irq(s->irq[i], 1);
>          }

With this change, if s->regs[GEM_ISR] is zero then the entire
function never does anything, so you could hoist that up to
the start, rather than testing it inside the loop and in the
previous conditional.

Also the comment says "raise or lower interrupt based on current
status", but the code will only ever do qemu_set_irq(..., 1),
never 0. Which is right?

thanks
-- PMM

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

[Peter Maydell]

On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Expose the Cadence GEM revision as a property.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  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 b9eaed4..7047e02 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];
> @@ -1213,7 +1215,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;
> @@ -1512,6 +1514,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
>
>

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

thanks
-- PMM

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

[Alistair Francis]

On Mon, Apr 10, 2017 at 5:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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 | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 17c229d..3e37665 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -481,14 +481,18 @@ 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 - busy buffer descriptor (q%d) 0x%x\n",
>> +                     i, s->rx_desc_addr[i]);
>
> This looks a little odd -- surely i isn't the right index to use
> into rx_desc_addr[] any more now we're outside the loop and i
> is always larger than the largest valid queue number? It looks
> like the debug print should be rephrased somehow.

Yeah you are right. It means that they are all busy, we can either
iterate over all of them and print out this or just print one
statement saying that. Somehow I ended up half way between both.

I'll update it to just print that they are all busy. I don't see why
we need every address printed.

Thanks,

Alistair

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Alistair Francis]

On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Only trigger multi-queue GEM interrupts if the interrupt status register
>> is set. This logic was already used for non multi-queue interrupts but
>> it also applies to multi-queue interrupts.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  hw/net/cadence_gem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 3e37665..b9eaed4 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
>>      }
>>
>>      for (i = 0; i < s->num_priority_queues; ++i) {
>> -        if (s->regs[GEM_INT_Q1_STATUS + i]) {
>> +        if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
>>              DB_PRINT("asserting int. (q=%d)\n", i);
>>              qemu_set_irq(s->irq[i], 1);
>>          }
>
> With this change, if s->regs[GEM_ISR] is zero then the entire
> function never does anything, so you could hoist that up to
> the start, rather than testing it inside the loop and in the
> previous conditional.

Yep, I have moved it to the top.

>
> Also the comment says "raise or lower interrupt based on current
> status", but the code will only ever do qemu_set_irq(..., 1),
> never 0. Which is right?

This is a little confusing. The interrupts are lowered when the ISR is
read, so the assumption was that we never need to clear them in the
gem_update_int_status(). Although then if we perform a reset nothing
will clear the interrupts until the ISR is read from. So that is a
bug. I'll update this patch to fix this up in V2.

Thanks,

Alistair

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Peter Maydell]

On 10 April 2017 at 23:23, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Also the comment says "raise or lower interrupt based on current
>> status", but the code will only ever do qemu_set_irq(..., 1),
>> never 0. Which is right?
>
> This is a little confusing. The interrupts are lowered when the ISR is
> read, so the assumption was that we never need to clear them in the
> gem_update_int_status(). Although then if we perform a reset nothing
> will clear the interrupts until the ISR is read from.

On QEMU reset the other end will be reset anyway so it will
update its idea of whether the irq is asserted (and calling
qemu_set_irq in a reset function is generally not a good idea).
If the device has guest-programmable reset of some kind you'd
need to clear the irq lines then, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/5] cadence_gem: Only trigger interrupts if the status register is set

[Alistair Francis]

On Tue, Apr 11, 2017 at 2:05 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 April 2017 at 23:23, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Also the comment says "raise or lower interrupt based on current
>>> status", but the code will only ever do qemu_set_irq(..., 1),
>>> never 0. Which is right?
>>
>> This is a little confusing. The interrupts are lowered when the ISR is
>> read, so the assumption was that we never need to clear them in the
>> gem_update_int_status(). Although then if we perform a reset nothing
>> will clear the interrupts until the ISR is read from.
>
> On QEMU reset the other end will be reset anyway so it will
> update its idea of whether the irq is asserted (and calling
> qemu_set_irq in a reset function is generally not a good idea).
> If the device has guest-programmable reset of some kind you'd
> need to clear the irq lines then, though.

Ok, that explains why it was always working.

I still like the idea of all irq updates (raise or lower) being done
in a single function. Instead of how it was previously where the
raises where in one function and the lower was in the read function.
That way if in some future IP version the ISR is cleared somewhere
else the interrupt logic is ready to handle it.

I did make one small error in my V2 though, so I'll send a V3 with the
consolidation still in it.

Thanks,

Alistair

>
> thanks
> -- PMM
>