[PATCH v2 00/10] Cadence GEM Fixes

[PATCH v2 00/10] Cadence GEM Fixes

[Sai Pavan Boddu]

Hi,

Following patch series fixes issues with priority queues,
Adds JUMBO Frame support,
Makes Debug statements compilable &
Fixes related to multicast frames.

Changes for V2:
	Fixed build failure on fedora docker machine
	Fix buggy debug print to use sized integer casting

Sai Pavan Boddu (9):
  net: cadence_gem: Fix debug statements
  net: cadence_gem: Fix the queue address update during wrap around
  net: cadence_gem: Fix irq update w.r.t queue
  net: cadence_gem: Define access permission for interrupt registers
  net: cadence_gem: Set ISR according to queue in use
  net: cadence_gem: Add support for jumbo frames
  net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg
  net: cadence_gem: Update the reset value for interrupt mask register
  net: cadence_gem: TX_LAST bit should be set by guest

Tong Ho (1):
  net: cadence_gem: Fix RX address filtering

 hw/net/cadence_gem.c | 167 +++++++++++++++++++++++++++++----------------------
 1 file changed, 94 insertions(+), 73 deletions(-)

-- 
2.7.4

[PATCH v2 01/10] net: cadence_gem: Fix debug statements

[Sai Pavan Boddu]

Enabling debug breaks the build, Fix them and make debug statements
always compilable. Fix few statements to use sized integer casting.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 22a0b1b..2f244eb 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -35,14 +35,13 @@
 #include "sysemu/dma.h"
 #include "net/checksum.h"
 
-#ifdef CADENCE_GEM_ERR_DEBUG
-#define DB_PRINT(...) do { \
-    fprintf(stderr,  ": %s: ", __func__); \
-    fprintf(stderr, ## __VA_ARGS__); \
-    } while (0)
-#else
-    #define DB_PRINT(...)
-#endif
+#define CADENCE_GEM_ERR_DEBUG 0
+#define DB_PRINT(...) do {\
+    if (CADENCE_GEM_ERR_DEBUG) {   \
+        qemu_log(": %s: ", __func__); \
+        qemu_log(__VA_ARGS__); \
+    } \
+} while (0)
 
 #define GEM_NWCTRL        (0x00000000/4) /* Network Control reg */
 #define GEM_NWCFG         (0x00000004/4) /* Network Config reg */
@@ -979,7 +978,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         size += 4;
     }
 
-    DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
+    DB_PRINT("config bufsize: %" PRIu64 " packet size: %" PRIu64 "\n",
+             (uint64_t) rxbufsize, (uint64_t) size);
 
     /* Find which queue we are targeting */
     q = get_queue_from_screen(s, rxbuf_ptr, rxbufsize);
@@ -992,9 +992,9 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
             return -1;
         }
 
-        DB_PRINT("copy %u bytes to 0x%" PRIx64 "\n",
-                 MIN(bytes_to_copy, rxbufsize),
-                 rx_desc_get_buffer(s, s->rx_desc[q]));
+        DB_PRINT("copy %" PRIu32 " bytes to 0x%" PRIx64 "\n",
+                MIN(bytes_to_copy, rxbufsize),
+                rx_desc_get_buffer(s, s->rx_desc[q] + rxbuf_offset));
 
         /* Copy packet data to emulated DMA buffer */
         address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
@@ -1160,8 +1160,8 @@ static void gem_transmit(CadenceGEMState *s)
              */
             if ((tx_desc_get_buffer(s, desc) == 0) ||
                 (tx_desc_get_length(desc) == 0)) {
-                DB_PRINT("Invalid TX descriptor @ 0x%x\n",
-                         (unsigned)packet_desc_addr);
+                DB_PRINT("Invalid TX descriptor @ 0x%" HWADDR_PRIx "\n",
+                         packet_desc_addr);
                 break;
             }
 
-- 
2.7.4

[PATCH v2 02/10] net: cadence_gem: Fix the queue address update during wrap around

[Sai Pavan Boddu]

During wrap around and reset, queues are pointing to initial base
address of queue 0, irrespective of what queue we are dealing with.
Fix it by assigning proper base address every time.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 2f244eb..6cb2f64 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -845,6 +845,25 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
     return 0;
 }
 
+static uint32_t gem_get_queue_base_addr(CadenceGEMState *s, bool tx, int q)
+{
+    uint32_t base_addr = 0;
+
+    switch (q) {
+    case 0:
+        base_addr = s->regs[tx ? GEM_TXQBASE : GEM_RXQBASE];
+        break;
+    case 1 ... (MAX_PRIORITY_QUEUES - 1):
+        base_addr = s->regs[(tx ? GEM_TRANSMIT_Q1_PTR :
+                                 GEM_RECEIVE_Q1_PTR) + q - 1];
+        break;
+    default:
+        g_assert_not_reached();
+    };
+
+    return base_addr;
+}
+
 static hwaddr gem_get_desc_addr(CadenceGEMState *s, bool tx, int q)
 {
     hwaddr desc_addr = 0;
@@ -1044,7 +1063,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
         /* Next descriptor */
         if (rx_desc_get_wrap(s->rx_desc[q])) {
             DB_PRINT("wrapping RX descriptor list\n");
-            s->rx_desc_addr[q] = s->regs[GEM_RXQBASE];
+            s->rx_desc_addr[q] = gem_get_queue_base_addr(s, false, q);
         } else {
             DB_PRINT("incrementing RX descriptor list\n");
             s->rx_desc_addr[q] += 4 * gem_get_desc_len(s, true);
@@ -1200,7 +1219,8 @@ static void gem_transmit(CadenceGEMState *s)
                                     sizeof(desc_first));
                 /* Advance the hardware current descriptor past this packet */
                 if (tx_desc_get_wrap(desc)) {
-                    s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
+                    s->tx_desc_addr[q] = gem_get_queue_base_addr(s,
+                                         true, q);
                 } else {
                     s->tx_desc_addr[q] = packet_desc_addr +
                                          4 * gem_get_desc_len(s, false);
@@ -1252,7 +1272,8 @@ static void gem_transmit(CadenceGEMState *s)
                 } else {
                     packet_desc_addr = 0;
                 }
-                packet_desc_addr |= s->regs[GEM_TXQBASE];
+                packet_desc_addr |= gem_get_queue_base_addr(s,
+                                    true, q);
             } else {
                 packet_desc_addr += 4 * gem_get_desc_len(s, false);
             }
@@ -1458,7 +1479,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         if (!(val & GEM_NWCTRL_TXENA)) {
             /* Reset to start of Q when transmit disabled. */
             for (i = 0; i < s->num_priority_queues; i++) {
-                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
+                s->tx_desc_addr[i] = gem_get_queue_base_addr(s, true, i);
             }
         }
         if (gem_can_receive(qemu_get_queue(s->nic))) {
-- 
2.7.4

[PATCH v2 03/10] net: cadence_gem: Fix irq update w.r.t queue

[Sai Pavan Boddu]

Set irq's specific to a queue, present implementation is setting q1 irq
based on q0 status.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 6cb2f64..a930bf1 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -554,29 +554,10 @@ static void gem_update_int_status(CadenceGEMState *s)
 {
     int i;
 
-    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;
-    }
+    qemu_set_irq(s->irq[0], !!s->regs[GEM_ISR]);
 
-    /* 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);
-        return;
-    }
-
-    for (i = 0; i < s->num_priority_queues; ++i) {
-        if (s->regs[GEM_INT_Q1_STATUS + i]) {
-            DB_PRINT("asserting int. (q=%d)\n", i);
-            qemu_set_irq(s->irq[i], 1);
-        }
+    for (i = 1; i < s->num_priority_queues; ++i) {
+        qemu_set_irq(s->irq[i], !!s->regs[GEM_INT_Q1_STATUS + i - 1]);
     }
 }
 
-- 
2.7.4

[PATCH v2 04/10] net: cadence_gem: Define access permission for interrupt registers

[Sai Pavan Boddu]

Q1 to Q7 ISR's are clear-on-read, IER/IDR registers
are write-only, mask reg are read-only.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index a930bf1..c532a14 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -458,6 +458,7 @@ static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
  */
 static void gem_init_register_masks(CadenceGEMState *s)
 {
+    unsigned int i;
     /* Mask of register bits which are read only */
     memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
     s->regs_ro[GEM_NWCTRL]   = 0xFFF80000;
@@ -470,10 +471,19 @@ static void gem_init_register_masks(CadenceGEMState *s)
     s->regs_ro[GEM_ISR]      = 0xFFFFFFFF;
     s->regs_ro[GEM_IMR]      = 0xFFFFFFFF;
     s->regs_ro[GEM_MODID]    = 0xFFFFFFFF;
+    for (i = 0; i < s->num_priority_queues; i++) {
+        s->regs_ro[GEM_INT_Q1_STATUS + i] = 0xFFFFFFFF;
+        s->regs_ro[GEM_INT_Q1_ENABLE + i] = 0xFFFFE319;
+        s->regs_ro[GEM_INT_Q1_DISABLE + i] = 0xFFFFE319;
+        s->regs_ro[GEM_INT_Q1_MASK + i] = 0xFFFFFFFF;
+    }
 
     /* Mask of register bits which are clear on read */
     memset(&s->regs_rtc[0], 0, sizeof(s->regs_rtc));
     s->regs_rtc[GEM_ISR]      = 0xFFFFFFFF;
+    for (i = 0; i < s->num_priority_queues; i++) {
+        s->regs_rtc[GEM_INT_Q1_STATUS + i] = 0x00000CE6;
+    }
 
     /* Mask of register bits which are write 1 to clear */
     memset(&s->regs_w1c[0], 0, sizeof(s->regs_w1c));
@@ -485,6 +495,10 @@ static void gem_init_register_masks(CadenceGEMState *s)
     s->regs_wo[GEM_NWCTRL]   = 0x00073E60;
     s->regs_wo[GEM_IER]      = 0x07FFFFFF;
     s->regs_wo[GEM_IDR]      = 0x07FFFFFF;
+    for (i = 0; i < s->num_priority_queues; i++) {
+        s->regs_wo[GEM_INT_Q1_ENABLE + i] = 0x00000CE6;
+        s->regs_wo[GEM_INT_Q1_DISABLE + i] = 0x00000CE6;
+    }
 }
 
 /*
-- 
2.7.4

[PATCH v2 05/10] net: cadence_gem: Set ISR according to queue in use

[Sai Pavan Boddu]

Set ISR according to queue in use, added interrupt support for
all queues.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index c532a14..beb38ec 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -896,7 +896,13 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
     if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
         DB_PRINT("descriptor 0x%" HWADDR_PRIx " owned by sw.\n", desc_addr);
         s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
-        s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
+        if (q == 0) {
+            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
+        } else {
+            s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXUSED &
+                                          ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
+        }
+
         /* Handle interrupt consequences */
         gem_update_int_status(s);
     }
@@ -1071,8 +1077,12 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     gem_receive_updatestats(s, buf, size);
 
     s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_FRMRCVD;
-    s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
-
+    if (q == 0) {
+        s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
+    } else {
+        s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXCMPL &
+                                      ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
+    }
     /* Handle interrupt consequences */
     gem_update_int_status(s);
 
@@ -1223,12 +1233,12 @@ static void gem_transmit(CadenceGEMState *s)
                 DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]);
 
                 s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
-                s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
-
+                if (q == 0) {
+                    s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
+                } else {
                 /* Update queue interrupt status */
-                if (s->num_priority_queues > 1) {
-                    s->regs[GEM_INT_Q1_STATUS + q] |=
-                            GEM_INT_TXCMPL & ~(s->regs[GEM_INT_Q1_MASK + q]);
+                    s->regs[GEM_INT_Q1_STATUS + q - 1] |=
+                            GEM_INT_TXCMPL & ~s->regs[GEM_INT_Q1_MASK + q - 1];
                 }
 
                 /* Handle interrupt consequences */
@@ -1280,7 +1290,10 @@ static void gem_transmit(CadenceGEMState *s)
 
         if (tx_desc_get_used(desc)) {
             s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
-            s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
+            /* IRQ TXUSED is defined only for queue 0 */
+            if (q == 0) {
+                s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
+            }
             gem_update_int_status(s);
         }
     }
-- 
2.7.4

[PATCH v2 06/10] net: cadence_gem: Add support for jumbo frames

[Sai Pavan Boddu]

Jumbo frames of size 10240 bytes is added.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index beb38ec..848be3f 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -313,6 +313,7 @@
 #define DESC_1_RX_EOF 0x00008000
 
 #define GEM_MODID_VALUE 0x00020118
+#define MAX_TX_FRAME_SIZE 10240
 
 static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
 {
@@ -1143,7 +1144,7 @@ static void gem_transmit(CadenceGEMState *s)
 {
     uint32_t desc[DESC_MAX_NUM_WORDS];
     hwaddr packet_desc_addr;
-    uint8_t     tx_packet[2048];
+    uint8_t     tx_packet[MAX_TX_FRAME_SIZE];
     uint8_t     *p;
     unsigned    total_bytes;
     int q = 0;
@@ -1344,7 +1345,7 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_RXPARTIALSF] = 0x000003ff;
     s->regs[GEM_MODID] = s->revision;
     s->regs[GEM_DESCONF] = 0x02500111;
-    s->regs[GEM_DESCONF2] = 0x2ab13fff;
+    s->regs[GEM_DESCONF2] = 0x2ab12800;
     s->regs[GEM_DESCONF5] = 0x002f2045;
     s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
 
-- 
2.7.4

[PATCH v2 07/10] net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg

[Sai Pavan Boddu]

Advertise support of clear-on-read for ISR registers.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@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 848be3f..9eb72a2 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1344,7 +1344,7 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_TXPARTIALSF] = 0x000003ff;
     s->regs[GEM_RXPARTIALSF] = 0x000003ff;
     s->regs[GEM_MODID] = s->revision;
-    s->regs[GEM_DESCONF] = 0x02500111;
+    s->regs[GEM_DESCONF] = 0x02D00111;
     s->regs[GEM_DESCONF2] = 0x2ab12800;
     s->regs[GEM_DESCONF5] = 0x002f2045;
     s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
-- 
2.7.4

[PATCH v2 08/10] net: cadence_gem: Update the reset value for interrupt mask register

[Sai Pavan Boddu]

Mask all interrupt on reset.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 9eb72a2..ac3a553 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -1348,6 +1348,7 @@ static void gem_reset(DeviceState *d)
     s->regs[GEM_DESCONF2] = 0x2ab12800;
     s->regs[GEM_DESCONF5] = 0x002f2045;
     s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
+    s->regs[GEM_INT_Q1_MASK] = 0x00000CE6;
 
     if (s->num_priority_queues > 1) {
         queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
-- 
2.7.4

[PATCH v2 09/10] net: cadence_gem: TX_LAST bit should be set by guest

[Sai Pavan Boddu]

TX_LAST bit should not be set by hardware, its set by guest to inform
the last bd of the frame.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/net/cadence_gem.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index ac3a553..f0bf2e7 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -345,11 +345,6 @@ static inline unsigned tx_desc_get_last(uint32_t *desc)
     return (desc[1] & DESC_1_TX_LAST) ? 1 : 0;
 }
 
-static inline void tx_desc_set_last(uint32_t *desc)
-{
-    desc[1] |= DESC_1_TX_LAST;
-}
-
 static inline unsigned tx_desc_get_length(uint32_t *desc)
 {
     return desc[1] & DESC_1_LENGTH;
@@ -1270,7 +1265,6 @@ static void gem_transmit(CadenceGEMState *s)
 
             /* read next descriptor */
             if (tx_desc_get_wrap(desc)) {
-                tx_desc_set_last(desc);
 
                 if (s->regs[GEM_DMACFG] & GEM_DMACFG_ADDR_64B) {
                     packet_desc_addr = s->regs[GEM_TBQPH];
-- 
2.7.4

[PATCH v2 10/10] net: cadence_gem: Fix RX address filtering

[Sai Pavan Boddu]

From: Tong Ho <tong.ho@xilinx.com>

Two defects are fixed:

1/ Detection of multicast frames
2/ Treating drop of mis-addressed frames as non-error

Signed-off-by: Tong Ho <tong.ho@xilinx.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 hw/net/cadence_gem.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index f0bf2e7..112794a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -34,6 +34,7 @@
 #include "qemu/module.h"
 #include "sysemu/dma.h"
 #include "net/checksum.h"
+#include "net/eth.h"
 
 #define CADENCE_GEM_ERR_DEBUG 0
 #define DB_PRINT(...) do {\
@@ -669,7 +670,7 @@ static unsigned calc_mac_hash(const uint8_t *mac)
 static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
 {
     uint8_t *gem_spaddr;
-    int i;
+    int i, is_mc;
 
     /* Promiscuous mode? */
     if (s->regs[GEM_NWCFG] & GEM_NWCFG_PROMISC) {
@@ -685,22 +686,17 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
     }
 
     /* Accept packets -w- hash match? */
-    if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
-        (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
+    is_mc = is_multicast_ether_addr(packet);
+    if ((is_mc && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
+        (!is_mc && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
+        uint64_t buckets;
         unsigned hash_index;
 
         hash_index = calc_mac_hash(packet);
-        if (hash_index < 32) {
-            if (s->regs[GEM_HASHLO] & (1<<hash_index)) {
-                return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
-                                           GEM_RX_UNICAST_HASH_ACCEPT;
-            }
-        } else {
-            hash_index -= 32;
-            if (s->regs[GEM_HASHHI] & (1<<hash_index)) {
-                return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
-                                           GEM_RX_UNICAST_HASH_ACCEPT;
-            }
+        buckets = ((uint64_t)s->regs[GEM_HASHHI] << 32) | s->regs[GEM_HASHLO];
+        if ((buckets >> hash_index) & 1) {
+            return is_mc ? GEM_RX_MULTICAST_HASH_ACCEPT
+                         : GEM_RX_UNICAST_HASH_ACCEPT;
         }
     }
 
@@ -924,7 +920,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     /* Is this destination MAC address "for us" ? */
     maf = gem_mac_address_filter(s, buf);
     if (maf == GEM_RX_REJECT) {
-        return -1;
+        return size;  /* no, drop siliently b/c it's not an error */
     }
 
     /* Discard packets with receive length error enabled ? */
-- 
2.7.4

Re: [PATCH v2 03/10] net: cadence_gem: Fix irq update w.r.t queue

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:01PM +0530, Sai Pavan Boddu wrote:
> Set irq's specific to a queue, present implementation is setting q1 irq
> based on q0 status.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 25 +++----------------------
>  1 file changed, 3 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 6cb2f64..a930bf1 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -554,29 +554,10 @@ static void gem_update_int_status(CadenceGEMState *s)
>  {
>      int i;
>  
> -    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;
> -    }
> +    qemu_set_irq(s->irq[0], !!s->regs[GEM_ISR]);
>  
> -    /* 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);
> -        return;
> -    }
> -
> -    for (i = 0; i < s->num_priority_queues; ++i) {
> -        if (s->regs[GEM_INT_Q1_STATUS + i]) {
> -            DB_PRINT("asserting int. (q=%d)\n", i);
> -            qemu_set_irq(s->irq[i], 1);
> -        }
> +    for (i = 1; i < s->num_priority_queues; ++i) {
> +        qemu_set_irq(s->irq[i], !!s->regs[GEM_INT_Q1_STATUS + i - 1]);
>      }
>  }
>  
> -- 
> 2.7.4
> 

Re: [PATCH v2 01/10] net: cadence_gem: Fix debug statements

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:35:59PM +0530, Sai Pavan Boddu wrote:
> Enabling debug breaks the build, Fix them and make debug statements
> always compilable. Fix few statements to use sized integer casting.
> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 22a0b1b..2f244eb 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -35,14 +35,13 @@
>  #include "sysemu/dma.h"
>  #include "net/checksum.h"
>  
> -#ifdef CADENCE_GEM_ERR_DEBUG
> -#define DB_PRINT(...) do { \
> -    fprintf(stderr,  ": %s: ", __func__); \
> -    fprintf(stderr, ## __VA_ARGS__); \
> -    } while (0)
> -#else
> -    #define DB_PRINT(...)
> -#endif
> +#define CADENCE_GEM_ERR_DEBUG 0
> +#define DB_PRINT(...) do {\
> +    if (CADENCE_GEM_ERR_DEBUG) {   \
> +        qemu_log(": %s: ", __func__); \
> +        qemu_log(__VA_ARGS__); \
> +    } \
> +} while (0)
>  
>  #define GEM_NWCTRL        (0x00000000/4) /* Network Control reg */
>  #define GEM_NWCFG         (0x00000004/4) /* Network Config reg */
> @@ -979,7 +978,8 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          size += 4;
>      }
>  
> -    DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
> +    DB_PRINT("config bufsize: %" PRIu64 " packet size: %" PRIu64 "\n",
> +             (uint64_t) rxbufsize, (uint64_t) size);

Shouldn't these be %u and %zd rather than casting to uint64_t?


>  
>      /* Find which queue we are targeting */
>      q = get_queue_from_screen(s, rxbuf_ptr, rxbufsize);
> @@ -992,9 +992,9 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>              return -1;
>          }
>  
> -        DB_PRINT("copy %u bytes to 0x%" PRIx64 "\n",
> -                 MIN(bytes_to_copy, rxbufsize),
> -                 rx_desc_get_buffer(s, s->rx_desc[q]));
> +        DB_PRINT("copy %" PRIu32 " bytes to 0x%" PRIx64 "\n",
> +                MIN(bytes_to_copy, rxbufsize),
> +                rx_desc_get_buffer(s, s->rx_desc[q] + rxbuf_offset));

Looks like this is changing what we print (+ rxbuf_offset), was
that intentional? (it was not mentioned in the commit message)


>  
>          /* Copy packet data to emulated DMA buffer */
>          address_space_write(&s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
> @@ -1160,8 +1160,8 @@ static void gem_transmit(CadenceGEMState *s)
>               */
>              if ((tx_desc_get_buffer(s, desc) == 0) ||
>                  (tx_desc_get_length(desc) == 0)) {
> -                DB_PRINT("Invalid TX descriptor @ 0x%x\n",
> -                         (unsigned)packet_desc_addr);
> +                DB_PRINT("Invalid TX descriptor @ 0x%" HWADDR_PRIx "\n",
> +                         packet_desc_addr);
>                  break;
>              }
>  
> -- 
> 2.7.4
> 

Re: [PATCH v2 02/10] net: cadence_gem: Fix the queue address update during wrap around

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:00PM +0530, Sai Pavan Boddu wrote:
> During wrap around and reset, queues are pointing to initial base
> address of queue 0, irrespective of what queue we are dealing with.
> Fix it by assigning proper base address every time.

Might want to add wrappers e.g:
static inline uint32_t gem_get_rx_queue_base_addr(CadenceGEMState *s, int q) {
    gem_get_queue_base_addr(s, false, q); 
}

static inline uint32_t gem_get_tx_queue_base_addr(CadenceGEMState *s, int q) {
    gem_get_queue_base_addr(s, true, q); 
}

It makes the packet processing logic a little easier to read, e.g:

s->rx_desc_addr[q] = gem_get_rx_queue_base_addr(s, q);
vs
s->rx_desc_addr[q] = gem_get_queue_base_addr(s, false, q);

Anyway, this looks good to me:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 2f244eb..6cb2f64 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -845,6 +845,25 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
>      return 0;
>  }
>  
> +static uint32_t gem_get_queue_base_addr(CadenceGEMState *s, bool tx, int q)
> +{
> +    uint32_t base_addr = 0;
> +
> +    switch (q) {
> +    case 0:
> +        base_addr = s->regs[tx ? GEM_TXQBASE : GEM_RXQBASE];
> +        break;
> +    case 1 ... (MAX_PRIORITY_QUEUES - 1):
> +        base_addr = s->regs[(tx ? GEM_TRANSMIT_Q1_PTR :
> +                                 GEM_RECEIVE_Q1_PTR) + q - 1];
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    };
> +
> +    return base_addr;
> +}
> +
>  static hwaddr gem_get_desc_addr(CadenceGEMState *s, bool tx, int q)
>  {
>      hwaddr desc_addr = 0;
> @@ -1044,7 +1063,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          /* Next descriptor */
>          if (rx_desc_get_wrap(s->rx_desc[q])) {
>              DB_PRINT("wrapping RX descriptor list\n");
> -            s->rx_desc_addr[q] = s->regs[GEM_RXQBASE];
> +            s->rx_desc_addr[q] = gem_get_queue_base_addr(s, false, q);
>          } else {
>              DB_PRINT("incrementing RX descriptor list\n");
>              s->rx_desc_addr[q] += 4 * gem_get_desc_len(s, true);
> @@ -1200,7 +1219,8 @@ static void gem_transmit(CadenceGEMState *s)
>                                      sizeof(desc_first));
>                  /* Advance the hardware current descriptor past this packet */
>                  if (tx_desc_get_wrap(desc)) {
> -                    s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
> +                    s->tx_desc_addr[q] = gem_get_queue_base_addr(s,
> +                                         true, q);
>                  } else {
>                      s->tx_desc_addr[q] = packet_desc_addr +
>                                           4 * gem_get_desc_len(s, false);
> @@ -1252,7 +1272,8 @@ static void gem_transmit(CadenceGEMState *s)
>                  } else {
>                      packet_desc_addr = 0;
>                  }
> -                packet_desc_addr |= s->regs[GEM_TXQBASE];
> +                packet_desc_addr |= gem_get_queue_base_addr(s,
> +                                    true, q);
>              } else {
>                  packet_desc_addr += 4 * gem_get_desc_len(s, false);
>              }
> @@ -1458,7 +1479,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>          if (!(val & GEM_NWCTRL_TXENA)) {
>              /* Reset to start of Q when transmit disabled. */
>              for (i = 0; i < s->num_priority_queues; i++) {
> -                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
> +                s->tx_desc_addr[i] = gem_get_queue_base_addr(s, true, i);
>              }
>          }
>          if (gem_can_receive(qemu_get_queue(s->nic))) {
> -- 
> 2.7.4
> 

Re: [PATCH v2 04/10] net: cadence_gem: Define access permission for interrupt registers

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:02PM +0530, Sai Pavan Boddu wrote:
> Q1 to Q7 ISR's are clear-on-read, IER/IDR registers
> are write-only, mask reg are read-only.
> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index a930bf1..c532a14 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -458,6 +458,7 @@ static const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
>   */
>  static void gem_init_register_masks(CadenceGEMState *s)
>  {
> +    unsigned int i;
>      /* Mask of register bits which are read only */
>      memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
>      s->regs_ro[GEM_NWCTRL]   = 0xFFF80000;
> @@ -470,10 +471,19 @@ static void gem_init_register_masks(CadenceGEMState *s)
>      s->regs_ro[GEM_ISR]      = 0xFFFFFFFF;
>      s->regs_ro[GEM_IMR]      = 0xFFFFFFFF;
>      s->regs_ro[GEM_MODID]    = 0xFFFFFFFF;
> +    for (i = 0; i < s->num_priority_queues; i++) {
> +        s->regs_ro[GEM_INT_Q1_STATUS + i] = 0xFFFFFFFF;
> +        s->regs_ro[GEM_INT_Q1_ENABLE + i] = 0xFFFFE319;
> +        s->regs_ro[GEM_INT_Q1_DISABLE + i] = 0xFFFFE319;

Shouldn't these be 0xfffff319?
Perhaps I'm looking at old specs but mine says bits upper bits [31:12]
are reserved and read-only.


With that fixed:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>





> +        s->regs_ro[GEM_INT_Q1_MASK + i] = 0xFFFFFFFF;

> +    }
>  
>      /* Mask of register bits which are clear on read */
>      memset(&s->regs_rtc[0], 0, sizeof(s->regs_rtc));
>      s->regs_rtc[GEM_ISR]      = 0xFFFFFFFF;
> +    for (i = 0; i < s->num_priority_queues; i++) {
> +        s->regs_rtc[GEM_INT_Q1_STATUS + i] = 0x00000CE6;
> +    }
>  
>      /* Mask of register bits which are write 1 to clear */
>      memset(&s->regs_w1c[0], 0, sizeof(s->regs_w1c));
> @@ -485,6 +495,10 @@ static void gem_init_register_masks(CadenceGEMState *s)
>      s->regs_wo[GEM_NWCTRL]   = 0x00073E60;
>      s->regs_wo[GEM_IER]      = 0x07FFFFFF;
>      s->regs_wo[GEM_IDR]      = 0x07FFFFFF;
> +    for (i = 0; i < s->num_priority_queues; i++) {
> +        s->regs_wo[GEM_INT_Q1_ENABLE + i] = 0x00000CE6;
> +        s->regs_wo[GEM_INT_Q1_DISABLE + i] = 0x00000CE6;
> +    }
>  }
>  
>  /*
> -- 
> 2.7.4
> 

Re: [PATCH v2 05/10] net: cadence_gem: Set ISR according to queue in use

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:03PM +0530, Sai Pavan Boddu wrote:
> Set ISR according to queue in use, added interrupt support for
> all queues.

Would it help to add a gem_set_isr(CadenceGEMState *s, int q, uint32_t flag) ?
Instead of open coding these if (q == 0) else... all over the place...

Anyway, the logic looks good to me:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index c532a14..beb38ec 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -896,7 +896,13 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
>      if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
>          DB_PRINT("descriptor 0x%" HWADDR_PRIx " owned by sw.\n", desc_addr);
>          s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
> -        s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> +        if (q == 0) {
> +            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> +        } else {
> +            s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXUSED &
> +                                          ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
> +        }
> +
>          /* Handle interrupt consequences */
>          gem_update_int_status(s);
>      }
> @@ -1071,8 +1077,12 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      gem_receive_updatestats(s, buf, size);
>  
>      s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_FRMRCVD;
> -    s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
> -
> +    if (q == 0) {
> +        s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
> +    } else {
> +        s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXCMPL &
> +                                      ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
> +    }
>      /* Handle interrupt consequences */
>      gem_update_int_status(s);
>  
> @@ -1223,12 +1233,12 @@ static void gem_transmit(CadenceGEMState *s)
>                  DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]);
>  
>                  s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
> -                s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
> -
> +                if (q == 0) {
> +                    s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
> +                } else {
>                  /* Update queue interrupt status */
> -                if (s->num_priority_queues > 1) {
> -                    s->regs[GEM_INT_Q1_STATUS + q] |=
> -                            GEM_INT_TXCMPL & ~(s->regs[GEM_INT_Q1_MASK + q]);
> +                    s->regs[GEM_INT_Q1_STATUS + q - 1] |=
> +                            GEM_INT_TXCMPL & ~s->regs[GEM_INT_Q1_MASK + q - 1];
>                  }
>  
>                  /* Handle interrupt consequences */
> @@ -1280,7 +1290,10 @@ static void gem_transmit(CadenceGEMState *s)
>  
>          if (tx_desc_get_used(desc)) {
>              s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
> -            s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
> +            /* IRQ TXUSED is defined only for queue 0 */
> +            if (q == 0) {
> +                s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
> +            }
>              gem_update_int_status(s);
>          }
>      }
> -- 
> 2.7.4
> 

Re: [PATCH v2 06/10] net: cadence_gem: Add support for jumbo frames

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:04PM +0530, Sai Pavan Boddu wrote:
> Jumbo frames of size 10240 bytes is added.

Hi Sai,

I think we should make this a property since it's a design
configuration option (10240 being the default).

> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index beb38ec..848be3f 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -313,6 +313,7 @@
>  #define DESC_1_RX_EOF 0x00008000
>  
>  #define GEM_MODID_VALUE 0x00020118
> +#define MAX_TX_FRAME_SIZE 10240

This applies to RX aswell, better to rename to MAX_FRAME_SIZE.

>  
>  static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
>  {
> @@ -1143,7 +1144,7 @@ static void gem_transmit(CadenceGEMState *s)
>  {
>      uint32_t desc[DESC_MAX_NUM_WORDS];
>      hwaddr packet_desc_addr;
> -    uint8_t     tx_packet[2048];
> +    uint8_t     tx_packet[MAX_TX_FRAME_SIZE];

rxbuf in gem_receive needs the same.
We also may want to consider moving these buffers from the stack
to CadenceGEMState *s.


>      uint8_t     *p;
>      unsigned    total_bytes;
>      int q = 0;
> @@ -1344,7 +1345,7 @@ static void gem_reset(DeviceState *d)
>      s->regs[GEM_RXPARTIALSF] = 0x000003ff;
>      s->regs[GEM_MODID] = s->revision;
>      s->regs[GEM_DESCONF] = 0x02500111;
> -    s->regs[GEM_DESCONF2] = 0x2ab13fff;
> +    s->regs[GEM_DESCONF2] = 0x2ab12800;


We need to add and populate the following register:
#define GEM_JUMBO_MAX_LEN   (0x00000048/4) /* Maximum Jumbo Frame Size */


>      s->regs[GEM_DESCONF5] = 0x002f2045;
>      s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
>  
> -- 
> 2.7.4
> 

Re: [PATCH v2 07/10] net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:05PM +0530, Sai Pavan Boddu wrote:
> Advertise support of clear-on-read for ISR registers.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@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 848be3f..9eb72a2 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1344,7 +1344,7 @@ static void gem_reset(DeviceState *d)
>      s->regs[GEM_TXPARTIALSF] = 0x000003ff;
>      s->regs[GEM_RXPARTIALSF] = 0x000003ff;
>      s->regs[GEM_MODID] = s->revision;
> -    s->regs[GEM_DESCONF] = 0x02500111;
> +    s->regs[GEM_DESCONF] = 0x02D00111;
>      s->regs[GEM_DESCONF2] = 0x2ab12800;
>      s->regs[GEM_DESCONF5] = 0x002f2045;
>      s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
> -- 
> 2.7.4
> 

Re: [PATCH v2 08/10] net: cadence_gem: Update the reset value for interrupt mask register

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:06PM +0530, Sai Pavan Boddu wrote:
> Mask all interrupt on reset.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 9eb72a2..ac3a553 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -1348,6 +1348,7 @@ static void gem_reset(DeviceState *d)
>      s->regs[GEM_DESCONF2] = 0x2ab12800;
>      s->regs[GEM_DESCONF5] = 0x002f2045;
>      s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
> +    s->regs[GEM_INT_Q1_MASK] = 0x00000CE6;
>  
>      if (s->num_priority_queues > 1) {
>          queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
> -- 
> 2.7.4
> 

Re: [PATCH v2 09/10] net: cadence_gem: TX_LAST bit should be set by guest

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:07PM +0530, Sai Pavan Boddu wrote:
> TX_LAST bit should not be set by hardware, its set by guest to inform
> the last bd of the frame.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index ac3a553..f0bf2e7 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -345,11 +345,6 @@ static inline unsigned tx_desc_get_last(uint32_t *desc)
>      return (desc[1] & DESC_1_TX_LAST) ? 1 : 0;
>  }
>  
> -static inline void tx_desc_set_last(uint32_t *desc)
> -{
> -    desc[1] |= DESC_1_TX_LAST;
> -}
> -
>  static inline unsigned tx_desc_get_length(uint32_t *desc)
>  {
>      return desc[1] & DESC_1_LENGTH;
> @@ -1270,7 +1265,6 @@ static void gem_transmit(CadenceGEMState *s)
>  
>              /* read next descriptor */
>              if (tx_desc_get_wrap(desc)) {
> -                tx_desc_set_last(desc);
>  
>                  if (s->regs[GEM_DMACFG] & GEM_DMACFG_ADDR_64B) {
>                      packet_desc_addr = s->regs[GEM_TBQPH];
> -- 
> 2.7.4
> 

Re: [PATCH v2 10/10] net: cadence_gem: Fix RX address filtering

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:08PM +0530, Sai Pavan Boddu wrote:
> From: Tong Ho <tong.ho@xilinx.com>
> 
> Two defects are fixed:
> 
> 1/ Detection of multicast frames
> 2/ Treating drop of mis-addressed frames as non-error


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Tong Ho <tong.ho@xilinx.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index f0bf2e7..112794a 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -34,6 +34,7 @@
>  #include "qemu/module.h"
>  #include "sysemu/dma.h"
>  #include "net/checksum.h"
> +#include "net/eth.h"
>  
>  #define CADENCE_GEM_ERR_DEBUG 0
>  #define DB_PRINT(...) do {\
> @@ -669,7 +670,7 @@ static unsigned calc_mac_hash(const uint8_t *mac)
>  static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
>  {
>      uint8_t *gem_spaddr;
> -    int i;
> +    int i, is_mc;
>  
>      /* Promiscuous mode? */
>      if (s->regs[GEM_NWCFG] & GEM_NWCFG_PROMISC) {
> @@ -685,22 +686,17 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
>      }
>  
>      /* Accept packets -w- hash match? */
> -    if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
> -        (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
> +    is_mc = is_multicast_ether_addr(packet);
> +    if ((is_mc && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
> +        (!is_mc && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
> +        uint64_t buckets;
>          unsigned hash_index;
>  
>          hash_index = calc_mac_hash(packet);
> -        if (hash_index < 32) {
> -            if (s->regs[GEM_HASHLO] & (1<<hash_index)) {
> -                return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
> -                                           GEM_RX_UNICAST_HASH_ACCEPT;
> -            }
> -        } else {
> -            hash_index -= 32;
> -            if (s->regs[GEM_HASHHI] & (1<<hash_index)) {
> -                return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
> -                                           GEM_RX_UNICAST_HASH_ACCEPT;
> -            }
> +        buckets = ((uint64_t)s->regs[GEM_HASHHI] << 32) | s->regs[GEM_HASHLO];
> +        if ((buckets >> hash_index) & 1) {
> +            return is_mc ? GEM_RX_MULTICAST_HASH_ACCEPT
> +                         : GEM_RX_UNICAST_HASH_ACCEPT;
>          }
>      }
>  
> @@ -924,7 +920,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      /* Is this destination MAC address "for us" ? */
>      maf = gem_mac_address_filter(s, buf);
>      if (maf == GEM_RX_REJECT) {
> -        return -1;
> +        return size;  /* no, drop siliently b/c it's not an error */
>      }
>  
>      /* Discard packets with receive length error enabled ? */
> -- 
> 2.7.4
> 

Re: [PATCH v2 00/10] Cadence GEM Fixes

[Ramon Fried]

On Mon, May 4, 2020 at 5:14 PM Sai Pavan Boddu
<sai.pavan.boddu@xilinx.com> wrote:
>
> Hi,
>
> Following patch series fixes issues with priority queues,
> Adds JUMBO Frame support,
> Makes Debug statements compilable &
> Fixes related to multicast frames.
>
> Changes for V2:
>         Fixed build failure on fedora docker machine
>         Fix buggy debug print to use sized integer casting
>
> Sai Pavan Boddu (9):
>   net: cadence_gem: Fix debug statements
>   net: cadence_gem: Fix the queue address update during wrap around
>   net: cadence_gem: Fix irq update w.r.t queue
>   net: cadence_gem: Define access permission for interrupt registers
>   net: cadence_gem: Set ISR according to queue in use
>   net: cadence_gem: Add support for jumbo frames
>   net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg
>   net: cadence_gem: Update the reset value for interrupt mask register
>   net: cadence_gem: TX_LAST bit should be set by guest
>
> Tong Ho (1):
>   net: cadence_gem: Fix RX address filtering
>
>  hw/net/cadence_gem.c | 167 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 94 insertions(+), 73 deletions(-)
>
> --
> 2.7.4
>
Hey. did you test these with 64 descriptor addressing ?
I can test it for you if you need.
Thanks,
Ramon.

RE: [PATCH v2 00/10] Cadence GEM Fixes

[Sai Pavan Boddu]

Hi Ramon,

> -----Original Message-----
> From: Ramon Fried <rfried.dev@gmail.com>
> Sent: Monday, May 4, 2020 9:20 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>; Alistair Francis
> <Alistair.Francis@wdc.com>; Peter Maydell <peter.maydell@linaro.org>;
> Jason Wang <jasowang@redhat.com>; Markus Armbruster
> <armbru@redhat.com>; Philippe Mathieu-Daudé <philmd@redhat.com>;
> Tong Ho <tongh@xilinx.com>; open list:Xilinx Zynq <qemu-
> arm@nongnu.org>; QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH v2 00/10] Cadence GEM Fixes
> 
> On Mon, May 4, 2020 at 5:14 PM Sai Pavan Boddu
> <sai.pavan.boddu@xilinx.com> wrote:
> >
> > Hi,
> >
> > Following patch series fixes issues with priority queues, Adds JUMBO
> > Frame support, Makes Debug statements compilable & Fixes related to
> > multicast frames.
> >
> > Changes for V2:
> >         Fixed build failure on fedora docker machine
> >         Fix buggy debug print to use sized integer casting
> >
> > Sai Pavan Boddu (9):
> >   net: cadence_gem: Fix debug statements
> >   net: cadence_gem: Fix the queue address update during wrap around
> >   net: cadence_gem: Fix irq update w.r.t queue
> >   net: cadence_gem: Define access permission for interrupt registers
> >   net: cadence_gem: Set ISR according to queue in use
> >   net: cadence_gem: Add support for jumbo frames
> >   net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg
> >   net: cadence_gem: Update the reset value for interrupt mask register
> >   net: cadence_gem: TX_LAST bit should be set by guest
> >
> > Tong Ho (1):
> >   net: cadence_gem: Fix RX address filtering
> >
> >  hw/net/cadence_gem.c | 167
> > +++++++++++++++++++++++++++++----------------------
> >  1 file changed, 94 insertions(+), 73 deletions(-)
> >
> > --
> > 2.7.4
> >
> Hey. did you test these with 64 descriptor addressing ?
[Sai Pavan Boddu] Tested it with a BareMetal application.

> I can test it for you if you need.
[Sai Pavan Boddu] Yes, would be nice, if you can review.

Thanks,
Sai Pavan
> Thanks,
> Ramon.

Re: [PATCH v2 00/10] Cadence GEM Fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/1588601168-27576-1-git-send-email-sai.pavan.boddu@xilinx.com/



Hi,

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

Message-id: 1588601168-27576-1-git-send-email-sai.pavan.boddu@xilinx.com
Subject: [PATCH v2 00/10] Cadence GEM Fixes
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
fatal: unable to write new index file
Traceback (most recent call last):
  File "patchew-tester/src/patchew-cli", line 521, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester/src/patchew-cli", line 57, in git_clone_repo
    cwd=clone)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'checkout', 'refs/tags/patchew/1588601168-27576-1-git-send-email-sai.pavan.boddu@xilinx.com', '-b', 'test']' returned non-zero exit status 128.



The full log is available at
http://patchew.org/logs/1588601168-27576-1-git-send-email-sai.pavan.boddu@xilinx.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

RE: [PATCH v2 01/10] net: cadence_gem: Fix debug statements

[Sai Pavan Boddu]

Hi Edgar,

Below comments will be taken care in V3.

Thanks,
Sai Pavan

> -----Original Message-----
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Sent: Monday, May 4, 2020 8:09 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell
> <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus
> Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried
> <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org
> Subject: Re: [PATCH v2 01/10] net: cadence_gem: Fix debug statements
> 
> On Mon, May 04, 2020 at 07:35:59PM +0530, Sai Pavan Boddu wrote:
> > Enabling debug breaks the build, Fix them and make debug statements
> > always compilable. Fix few statements to use sized integer casting.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/net/cadence_gem.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > 22a0b1b..2f244eb 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -35,14 +35,13 @@
> >  #include "sysemu/dma.h"
> >  #include "net/checksum.h"
> >
> > -#ifdef CADENCE_GEM_ERR_DEBUG
> > -#define DB_PRINT(...) do { \
> > -    fprintf(stderr,  ": %s: ", __func__); \
> > -    fprintf(stderr, ## __VA_ARGS__); \
> > -    } while (0)
> > -#else
> > -    #define DB_PRINT(...)
> > -#endif
> > +#define CADENCE_GEM_ERR_DEBUG 0
> > +#define DB_PRINT(...) do {\
> > +    if (CADENCE_GEM_ERR_DEBUG) {   \
> > +        qemu_log(": %s: ", __func__); \
> > +        qemu_log(__VA_ARGS__); \
> > +    } \
> > +} while (0)
> >
> >  #define GEM_NWCTRL        (0x00000000/4) /* Network Control reg */
> >  #define GEM_NWCFG         (0x00000004/4) /* Network Config reg */
> > @@ -979,7 +978,8 @@ static ssize_t gem_receive(NetClientState *nc,
> const uint8_t *buf, size_t size)
> >          size += 4;
> >      }
> >
> > -    DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
> > +    DB_PRINT("config bufsize: %" PRIu64 " packet size: %" PRIu64 "\n",
> > +             (uint64_t) rxbufsize, (uint64_t) size);
> 
> Shouldn't these be %u and %zd rather than casting to uint64_t?
> 
> 
> >
> >      /* Find which queue we are targeting */
> >      q = get_queue_from_screen(s, rxbuf_ptr, rxbufsize); @@ -992,9
> > +992,9 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t
> *buf, size_t size)
> >              return -1;
> >          }
> >
> > -        DB_PRINT("copy %u bytes to 0x%" PRIx64 "\n",
> > -                 MIN(bytes_to_copy, rxbufsize),
> > -                 rx_desc_get_buffer(s, s->rx_desc[q]));
> > +        DB_PRINT("copy %" PRIu32 " bytes to 0x%" PRIx64 "\n",
> > +                MIN(bytes_to_copy, rxbufsize),
> > +                rx_desc_get_buffer(s, s->rx_desc[q] + rxbuf_offset));
> 
> Looks like this is changing what we print (+ rxbuf_offset), was that
> intentional? (it was not mentioned in the commit message)
> 
> 
> >
> >          /* Copy packet data to emulated DMA buffer */
> >          address_space_write(&s->dma_as, rx_desc_get_buffer(s,
> > s->rx_desc[q]) + @@ -1160,8 +1160,8 @@ static void
> gem_transmit(CadenceGEMState *s)
> >               */
> >              if ((tx_desc_get_buffer(s, desc) == 0) ||
> >                  (tx_desc_get_length(desc) == 0)) {
> > -                DB_PRINT("Invalid TX descriptor @ 0x%x\n",
> > -                         (unsigned)packet_desc_addr);
> > +                DB_PRINT("Invalid TX descriptor @ 0x%" HWADDR_PRIx "\n",
> > +                         packet_desc_addr);
> >                  break;
> >              }
> >
> > --
> > 2.7.4
> >

RE: [PATCH v2 04/10] net: cadence_gem: Define access permission for interrupt registers

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Sent: Monday, May 4, 2020 8:27 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell
> <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus
> Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried
> <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org
> Subject: Re: [PATCH v2 04/10] net: cadence_gem: Define access permission
> for interrupt registers
> 
> On Mon, May 04, 2020 at 07:36:02PM +0530, Sai Pavan Boddu wrote:
> > Q1 to Q7 ISR's are clear-on-read, IER/IDR registers are write-only,
> > mask reg are read-only.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/net/cadence_gem.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > a930bf1..c532a14 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -458,6 +458,7 @@ static const uint8_t broadcast_addr[] = { 0xFF, 0xFF,
> 0xFF, 0xFF, 0xFF, 0xFF };
> >   */
> >  static void gem_init_register_masks(CadenceGEMState *s)  {
> > +    unsigned int i;
> >      /* Mask of register bits which are read only */
> >      memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
> >      s->regs_ro[GEM_NWCTRL]   = 0xFFF80000;
> > @@ -470,10 +471,19 @@ static void
> gem_init_register_masks(CadenceGEMState *s)
> >      s->regs_ro[GEM_ISR]      = 0xFFFFFFFF;
> >      s->regs_ro[GEM_IMR]      = 0xFFFFFFFF;
> >      s->regs_ro[GEM_MODID]    = 0xFFFFFFFF;
> > +    for (i = 0; i < s->num_priority_queues; i++) {
> > +        s->regs_ro[GEM_INT_Q1_STATUS + i] = 0xFFFFFFFF;
> > +        s->regs_ro[GEM_INT_Q1_ENABLE + i] = 0xFFFFE319;
> > +        s->regs_ro[GEM_INT_Q1_DISABLE + i] = 0xFFFFE319;
> 
> Shouldn't these be 0xfffff319?
[Sai Pavan Boddu] This one is right. I would fix it thanks.

Regards,
Sai Pavan
> Perhaps I'm looking at old specs but mine says bits upper bits [31:12] are
> reserved and read-only.
> 
> 
> With that fixed:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> 
> 
> 
> 
> > +        s->regs_ro[GEM_INT_Q1_MASK + i] = 0xFFFFFFFF;
> 
> > +    }
> >
> >      /* Mask of register bits which are clear on read */
> >      memset(&s->regs_rtc[0], 0, sizeof(s->regs_rtc));
> >      s->regs_rtc[GEM_ISR]      = 0xFFFFFFFF;
> > +    for (i = 0; i < s->num_priority_queues; i++) {
> > +        s->regs_rtc[GEM_INT_Q1_STATUS + i] = 0x00000CE6;
> > +    }
> >
> >      /* Mask of register bits which are write 1 to clear */
> >      memset(&s->regs_w1c[0], 0, sizeof(s->regs_w1c)); @@ -485,6
> > +495,10 @@ static void gem_init_register_masks(CadenceGEMState *s)
> >      s->regs_wo[GEM_NWCTRL]   = 0x00073E60;
> >      s->regs_wo[GEM_IER]      = 0x07FFFFFF;
> >      s->regs_wo[GEM_IDR]      = 0x07FFFFFF;
> > +    for (i = 0; i < s->num_priority_queues; i++) {
> > +        s->regs_wo[GEM_INT_Q1_ENABLE + i] = 0x00000CE6;
> > +        s->regs_wo[GEM_INT_Q1_DISABLE + i] = 0x00000CE6;
> > +    }
> >  }
> >
> >  /*
> > --
> > 2.7.4
> >

RE: [PATCH v2 05/10] net: cadence_gem: Set ISR according to queue in use

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Sent: Monday, May 4, 2020 8:32 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell
> <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus
> Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried
> <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org
> Subject: Re: [PATCH v2 05/10] net: cadence_gem: Set ISR according to queue
> in use
> 
> On Mon, May 04, 2020 at 07:36:03PM +0530, Sai Pavan Boddu wrote:
> > Set ISR according to queue in use, added interrupt support for all
> > queues.
> 
> Would it help to add a gem_set_isr(CadenceGEMState *s, int q, uint32_t
> flag) ?
> Instead of open coding these if (q == 0) else... all over the place...
[Sai Pavan Boddu] Yeah, it would be nice. Will try to include this in v3

Thanks,
Sai Pavan
> 
> Anyway, the logic looks good to me:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> 
> 
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  hw/net/cadence_gem.c | 31 ++++++++++++++++++++++---------
> >  1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > c532a14..beb38ec 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -896,7 +896,13 @@ static void gem_get_rx_desc(CadenceGEMState
> *s, int q)
> >      if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
> >          DB_PRINT("descriptor 0x%" HWADDR_PRIx " owned by sw.\n",
> desc_addr);
> >          s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
> > -        s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> > +        if (q == 0) {
> > +            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> > +        } else {
> > +            s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXUSED &
> > +                                          ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
> > +        }
> > +
> >          /* Handle interrupt consequences */
> >          gem_update_int_status(s);
> >      }
> > @@ -1071,8 +1077,12 @@ static ssize_t gem_receive(NetClientState *nc,
> const uint8_t *buf, size_t size)
> >      gem_receive_updatestats(s, buf, size);
> >
> >      s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_FRMRCVD;
> > -    s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
> > -
> > +    if (q == 0) {
> > +        s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
> > +    } else {
> > +        s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXCMPL &
> > +                                      ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
> > +    }
> >      /* Handle interrupt consequences */
> >      gem_update_int_status(s);
> >
> > @@ -1223,12 +1233,12 @@ static void gem_transmit(CadenceGEMState
> *s)
> >                  DB_PRINT("TX descriptor next: 0x%08x\n",
> > s->tx_desc_addr[q]);
> >
> >                  s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
> > -                s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
> > -
> > +                if (q == 0) {
> > +                    s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s-
> >regs[GEM_IMR]);
> > +                } else {
> >                  /* Update queue interrupt status */
> > -                if (s->num_priority_queues > 1) {
> > -                    s->regs[GEM_INT_Q1_STATUS + q] |=
> > -                            GEM_INT_TXCMPL & ~(s->regs[GEM_INT_Q1_MASK + q]);
> > +                    s->regs[GEM_INT_Q1_STATUS + q - 1] |=
> > +                            GEM_INT_TXCMPL & ~s->regs[GEM_INT_Q1_MASK
> > + + q - 1];
> >                  }
> >
> >                  /* Handle interrupt consequences */ @@ -1280,7
> > +1290,10 @@ static void gem_transmit(CadenceGEMState *s)
> >
> >          if (tx_desc_get_used(desc)) {
> >              s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
> > -            s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
> > +            /* IRQ TXUSED is defined only for queue 0 */
> > +            if (q == 0) {
> > +                s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s-
> >regs[GEM_IMR]);
> > +            }
> >              gem_update_int_status(s);
> >          }
> >      }
> > --
> > 2.7.4
> >