diff mbox series

[v8,07/11] include/hw/net: Implemented Classes and Masks for GMAC Descriptors

Message ID 20231214211527.1946302-8-nabihestefan@google.com
State New
Headers show
Series Implementation of NPI Mailbox and GMAC Networking Module | expand

Commit Message

Nabih Estefan Dec. 14, 2023, 9:15 p.m. UTC 
From: Nabih Estefan Diaz <nabihestefan@google.com>

 - Implemeted classes for GMAC Receive and Transmit Descriptors
 - Implemented Masks for said descriptors

Signed-off-by: Nabih Estefan <nabihestefan@google.com>
Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
---
 hw/net/npcm_gmac.c           | 183 +++++++++++++++++++++++++++--------
 hw/net/trace-events          |   9 ++
 include/hw/net/npcm_gmac.h   |   2 -
 tests/qtest/npcm_gmac-test.c |   2 +-
 4 files changed, 150 insertions(+), 46 deletions(-)

Comments

Peter Maydell Dec. 18, 2023, 2:36 p.m. UTC | #1 
On Thu, 14 Dec 2023 at 21:15, Nabih Estefan <nabihestefan@google.com> wrote:
>
> From: Nabih Estefan Diaz <nabihestefan@google.com>
>
>  - Implemeted classes for GMAC Receive and Transmit Descriptors
>  - Implemented Masks for said descriptors
>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>


>  static const uint32_t npcm_gmac_cold_reset_values[NPCM_GMAC_NR_REGS] = {
> -    [R_NPCM_GMAC_VERSION]         = 0x00001037,
> +    /* Reduce version to 3.2 so that the kernel can enable interrupt. */
> +    [R_NPCM_GMAC_VERSION]         = 0x00001032,

This needs more description. What's going on here? Generally
workarounds for guest bugs are a bad idea, because once we put
them in we have pretty much no way of ever getting them out
of the codebase again. This goes doubly so if we don't record
exactly what the problem was that made us do the workaround...

Also, if you want to implement only v3.2, then do that from
the start, don't start with a patch that says "3.7" and then
change it in this patch.

>      [R_NPCM_GMAC_TIMER_CTRL]      = 0x03e80000,
>      [R_NPCM_GMAC_MAC0_ADDR_HI]    = 0x8000ffff,
>      [R_NPCM_GMAC_MAC0_ADDR_LO]    = 0xffffffff,
> @@ -125,12 +126,12 @@ static const uint16_t phy_reg_init[] = {
>      [MII_EXTSTAT]   = 0x3000, /* 1000BASTE_T full-duplex capable */
>  };
>
> -static void npcm_gmac_soft_reset(NPCMGMACState *s)
> +static void npcm_gmac_soft_reset(NPCMGMACState *gmac)
>  {
> -    memcpy(s->regs, npcm_gmac_cold_reset_values,
> +    memcpy(gmac->regs, npcm_gmac_cold_reset_values,
>             NPCM_GMAC_NR_REGS * sizeof(uint32_t));
>      /* Clear reset bits */
> -    s->regs[R_NPCM_DMA_BUS_MODE] &= ~NPCM_DMA_BUS_MODE_SWR;
> +    gmac->regs[R_NPCM_DMA_BUS_MODE] &= ~NPCM_DMA_BUS_MODE_SWR;
>  }

What is this churn from 's' to 'gmac' doing in this patch?
If you want 'gmac', that's fine, but this file is new in
this patch series, so just make it be the variable name you
want in the patch where you add this function, rather than
adding it with one name then changing it later in the series.

>  static void gmac_phy_set_link(NPCMGMACState *s, bool active)
> @@ -148,11 +149,53 @@ static bool gmac_can_receive(NetClientState *nc)
>      return true;
>  }
>
> -static ssize_t gmac_receive(NetClientState *nc, const uint8_t *buf, size_t len1)

Similarly, why did the 'len1' argument here change to 'len',
and the "/* Placeholder */" comment appear only now? The effect
is it looks like this patch changed that function, but it didn't
in any interesting way.

That comment is not very helpful, by the way -- if, eg, the
idea is that the function gets filled in later in the patchseries,
then say so. If the function is a dummy one because of a missing
feature that won't be added in this patchset, then say that. Etc.

> +/*
> + * Function that updates the GMAC IRQ
> + * It find the logical OR of the enabled bits for NIS (if enabled)
> + * It find the logical OR of the enabled bits for AIS (if enabled)
> + */
> +static void gmac_update_irq(NPCMGMACState *gmac)
>  {
> -    return 0;
> +    /*
> +     * Check if the normal interrupts summery is enabled

It might be helpful to run a spellcheck on your comments.
This is "summary" (which you get right on the line below).

> +     * if so, add the bits for the summary that are enabled
> +     */
> +    if (gmac->regs[R_NPCM_DMA_INTR_ENA] & gmac->regs[R_NPCM_DMA_STATUS] &
> +        (NPCM_DMA_INTR_ENAB_NIE_BITS))
> +    {
> +        gmac->regs[R_NPCM_DMA_STATUS] |=  NPCM_DMA_STATUS_NIS;
> +    }

Our coding style puts the opening brace on the same line as the if(),
not on a line of its own.

> +    /*
> +     * Check if the abnormal interrupts summery is enabled
> +     * if so, add the bits for the summary that are enabled
> +     */
> +    if (gmac->regs[R_NPCM_DMA_INTR_ENA] & gmac->regs[R_NPCM_DMA_STATUS] &
> +        (NPCM_DMA_INTR_ENAB_AIE_BITS))
> +    {
> +        gmac->regs[R_NPCM_DMA_STATUS] |=  NPCM_DMA_STATUS_AIS;
> +    }
> +
> +    /* Get the logical OR of both normal and abnormal interrupts */
> +    int level = !!((gmac->regs[R_NPCM_DMA_STATUS] &
> +                    gmac->regs[R_NPCM_DMA_INTR_ENA] &
> +                    NPCM_DMA_STATUS_NIS) |
> +                   (gmac->regs[R_NPCM_DMA_STATUS] &
> +                   gmac->regs[R_NPCM_DMA_INTR_ENA] &
> +                   NPCM_DMA_STATUS_AIS));
> +
> +    /* Set the IRQ */
> +    trace_npcm_gmac_update_irq(DEVICE(gmac)->canonical_path,
> +                               gmac->regs[R_NPCM_DMA_STATUS],
> +                               gmac->regs[R_NPCM_DMA_INTR_ENA],
> +                               level);
> +    qemu_set_irq(gmac->irq, level);
>  }
>
> +static ssize_t gmac_receive(NetClientState *nc, const uint8_t *buf, size_t len)
> +{
> +    /* Placeholder */
> +    return 0;
> +}
>  static void gmac_cleanup(NetClientState *nc)
>  {
>      /* Nothing to do yet. */
> @@ -166,7 +209,7 @@ static void gmac_set_link(NetClientState *nc)
>      gmac_phy_set_link(s, !nc->link_down);
>  }
>
> -static void npcm_gmac_mdio_access(NPCMGMACState *s, uint16_t v)
> +static void npcm_gmac_mdio_access(NPCMGMACState *gmac, uint16_t v)

More changes of variable names. Please clean all this up into
the right patches.

>  {
>      bool busy = v & NPCM_GMAC_MII_ADDR_BUSY;
>      uint8_t is_write;

> diff --git a/include/hw/net/npcm_gmac.h b/include/hw/net/npcm_gmac.h
> index a92a654278..e5729e83ea 100644
> --- a/include/hw/net/npcm_gmac.h
> +++ b/include/hw/net/npcm_gmac.h
> @@ -37,8 +37,6 @@ struct NPCMGMACRxDesc {
>  /* RDES2 and RDES3 are buffer address pointers */
>  /* Owner: 0 = software, 1 = gmac */
>  #define RX_DESC_RDES0_OWNER_MASK BIT(31)
> -/* Owner*/
> -#define RX_DESC_RDES0_OWNER_SHIFT 31

Why did this go away?

>  /* Destination Address Filter Fail */
>  #define RX_DESC_RDES0_DEST_ADDR_FILT_FAIL_MASK BIT(30)
>  /* Frame length*/
> diff --git a/tests/qtest/npcm_gmac-test.c b/tests/qtest/npcm_gmac-test.c
> index 77a83c4c58..130a1599a8 100644
> --- a/tests/qtest/npcm_gmac-test.c
> +++ b/tests/qtest/npcm_gmac-test.c
> @@ -154,7 +154,7 @@ static void test_init(gconstpointer test_data)
>      CHECK_REG32(NPCM_GMAC_MII_DATA, 0);
>      CHECK_REG32(NPCM_GMAC_FLOW_CTRL, 0);
>      CHECK_REG32(NPCM_GMAC_VLAN_FLAG, 0);
> -    CHECK_REG32(NPCM_GMAC_VERSION, 0x00001037);
> +    CHECK_REG32(NPCM_GMAC_VERSION, 0x00001032);
>      CHECK_REG32(NPCM_GMAC_WAKEUP_FILTER, 0);
>      CHECK_REG32(NPCM_GMAC_PMT, 0);
>      CHECK_REG32(NPCM_GMAC_LPI_CTRL, 0);
> --

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
index f26380ad7b..be3f076200 100644
--- a/hw/net/npcm_gmac.c
+++ b/hw/net/npcm_gmac.c
@@ -32,7 +32,7 @@ 
 REG32(NPCM_DMA_BUS_MODE, 0x1000)
 REG32(NPCM_DMA_XMT_POLL_DEMAND, 0x1004)
 REG32(NPCM_DMA_RCV_POLL_DEMAND, 0x1008)
-REG32(NPCM_DMA_RCV_BASE_ADDR, 0x100c)
+REG32(NPCM_DMA_RX_BASE_ADDR, 0x100c)
 REG32(NPCM_DMA_TX_BASE_ADDR, 0x1010)
 REG32(NPCM_DMA_STATUS, 0x1014)
 REG32(NPCM_DMA_CONTROL, 0x1018)
@@ -91,7 +91,8 @@  REG32(NPCM_GMAC_PTP_TTSR, 0x71c)
 #define NPCM_DMA_BUS_MODE_SWR               BIT(0)
 
 static const uint32_t npcm_gmac_cold_reset_values[NPCM_GMAC_NR_REGS] = {
-    [R_NPCM_GMAC_VERSION]         = 0x00001037,
+    /* Reduce version to 3.2 so that the kernel can enable interrupt. */
+    [R_NPCM_GMAC_VERSION]         = 0x00001032,
     [R_NPCM_GMAC_TIMER_CTRL]      = 0x03e80000,
     [R_NPCM_GMAC_MAC0_ADDR_HI]    = 0x8000ffff,
     [R_NPCM_GMAC_MAC0_ADDR_LO]    = 0xffffffff,
@@ -125,12 +126,12 @@  static const uint16_t phy_reg_init[] = {
     [MII_EXTSTAT]   = 0x3000, /* 1000BASTE_T full-duplex capable */
 };
 
-static void npcm_gmac_soft_reset(NPCMGMACState *s)
+static void npcm_gmac_soft_reset(NPCMGMACState *gmac)
 {
-    memcpy(s->regs, npcm_gmac_cold_reset_values,
+    memcpy(gmac->regs, npcm_gmac_cold_reset_values,
            NPCM_GMAC_NR_REGS * sizeof(uint32_t));
     /* Clear reset bits */
-    s->regs[R_NPCM_DMA_BUS_MODE] &= ~NPCM_DMA_BUS_MODE_SWR;
+    gmac->regs[R_NPCM_DMA_BUS_MODE] &= ~NPCM_DMA_BUS_MODE_SWR;
 }
 
 static void gmac_phy_set_link(NPCMGMACState *s, bool active)
@@ -148,11 +149,53 @@  static bool gmac_can_receive(NetClientState *nc)
     return true;
 }
 
-static ssize_t gmac_receive(NetClientState *nc, const uint8_t *buf, size_t len1)
+/*
+ * Function that updates the GMAC IRQ
+ * It find the logical OR of the enabled bits for NIS (if enabled)
+ * It find the logical OR of the enabled bits for AIS (if enabled)
+ */
+static void gmac_update_irq(NPCMGMACState *gmac)
 {
-    return 0;
+    /*
+     * Check if the normal interrupts summery is enabled
+     * if so, add the bits for the summary that are enabled
+     */
+    if (gmac->regs[R_NPCM_DMA_INTR_ENA] & gmac->regs[R_NPCM_DMA_STATUS] &
+        (NPCM_DMA_INTR_ENAB_NIE_BITS))
+    {
+        gmac->regs[R_NPCM_DMA_STATUS] |=  NPCM_DMA_STATUS_NIS;
+    }
+    /*
+     * Check if the abnormal interrupts summery is enabled
+     * if so, add the bits for the summary that are enabled
+     */
+    if (gmac->regs[R_NPCM_DMA_INTR_ENA] & gmac->regs[R_NPCM_DMA_STATUS] &
+        (NPCM_DMA_INTR_ENAB_AIE_BITS))
+    {
+        gmac->regs[R_NPCM_DMA_STATUS] |=  NPCM_DMA_STATUS_AIS;
+    }
+
+    /* Get the logical OR of both normal and abnormal interrupts */
+    int level = !!((gmac->regs[R_NPCM_DMA_STATUS] &
+                    gmac->regs[R_NPCM_DMA_INTR_ENA] &
+                    NPCM_DMA_STATUS_NIS) |
+                   (gmac->regs[R_NPCM_DMA_STATUS] &
+                   gmac->regs[R_NPCM_DMA_INTR_ENA] &
+                   NPCM_DMA_STATUS_AIS));
+
+    /* Set the IRQ */
+    trace_npcm_gmac_update_irq(DEVICE(gmac)->canonical_path,
+                               gmac->regs[R_NPCM_DMA_STATUS],
+                               gmac->regs[R_NPCM_DMA_INTR_ENA],
+                               level);
+    qemu_set_irq(gmac->irq, level);
 }
 
+static ssize_t gmac_receive(NetClientState *nc, const uint8_t *buf, size_t len)
+{
+    /* Placeholder */
+    return 0;
+}
 static void gmac_cleanup(NetClientState *nc)
 {
     /* Nothing to do yet. */
@@ -166,7 +209,7 @@  static void gmac_set_link(NetClientState *nc)
     gmac_phy_set_link(s, !nc->link_down);
 }
 
-static void npcm_gmac_mdio_access(NPCMGMACState *s, uint16_t v)
+static void npcm_gmac_mdio_access(NPCMGMACState *gmac, uint16_t v)
 {
     bool busy = v & NPCM_GMAC_MII_ADDR_BUSY;
     uint8_t is_write;
@@ -183,33 +226,38 @@  static void npcm_gmac_mdio_access(NPCMGMACState *s, uint16_t v)
 
 
         if (v & NPCM_GMAC_MII_ADDR_WRITE) {
-            data = s->regs[R_NPCM_GMAC_MII_DATA];
+            data = gmac->regs[R_NPCM_GMAC_MII_DATA];
             /* Clear reset bit for BMCR register */
             switch (gr) {
             case MII_BMCR:
                 data &= ~MII_BMCR_RESET;
-                /* Complete auto-negotiation immediately and set as complete */
-                if (data & MII_BMCR_AUTOEN) {
+                /* Autonegotiation is a W1C bit*/
+                if (data & MII_BMCR_ANRESTART) {
                     /* Tells autonegotiation to not restart again */
                     data &= ~MII_BMCR_ANRESTART;
+                }
+                if ((data & MII_BMCR_AUTOEN) &&
+                    !(gmac->phy_regs[pa][MII_BMSR] & MII_BMSR_AN_COMP)) {
                     /* sets autonegotiation as complete */
-                    s->phy_regs[pa][MII_BMSR] |= MII_BMSR_AN_COMP;
+                    gmac->phy_regs[pa][MII_BMSR] |= MII_BMSR_AN_COMP;
+                    /* Resolve AN automatically->need to set this */
+                    gmac->phy_regs[0][MII_ANLPAR] = 0x0000;
                 }
             }
-            s->phy_regs[pa][gr] = data;
+            gmac->phy_regs[pa][gr] = data;
         } else {
-            data = s->phy_regs[pa][gr];
-            s->regs[R_NPCM_GMAC_MII_DATA] = data;
+            data = gmac->phy_regs[pa][gr];
+            gmac->regs[R_NPCM_GMAC_MII_DATA] = data;
         }
-        trace_npcm_gmac_mdio_access(DEVICE(s)->canonical_path, is_write, pa,
-                                    gr, data);
+        trace_npcm_gmac_mdio_access(DEVICE(gmac)->canonical_path, is_write, pa,
+                                        gr, data);
     }
-    s->regs[R_NPCM_GMAC_MII_ADDR] = v & ~NPCM_GMAC_MII_ADDR_BUSY;
+    gmac->regs[R_NPCM_GMAC_MII_ADDR] = v & ~NPCM_GMAC_MII_ADDR_BUSY;
 }
 
 static uint64_t npcm_gmac_read(void *opaque, hwaddr offset, unsigned size)
 {
-    NPCMGMACState *s = opaque;
+    NPCMGMACState *gmac = opaque;
     uint32_t v = 0;
 
     switch (offset) {
@@ -218,22 +266,25 @@  static uint64_t npcm_gmac_read(void *opaque, hwaddr offset, unsigned size)
     case A_NPCM_DMA_RCV_POLL_DEMAND:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Read of write-only reg: offset: 0x%04" HWADDR_PRIx
-                      "\n", DEVICE(s)->canonical_path, offset);
+                      "\n", DEVICE(gmac)->canonical_path, offset);
         break;
 
     default:
-        v = s->regs[offset / sizeof(uint32_t)];
+        v = gmac->regs[offset / sizeof(uint32_t)];
     }
-    trace_npcm_gmac_reg_read(DEVICE(s)->canonical_path, offset, v);
+
+    trace_npcm_gmac_reg_read(DEVICE(gmac)->canonical_path, offset, v);
     return v;
 }
 
 static void npcm_gmac_write(void *opaque, hwaddr offset,
                               uint64_t v, unsigned size)
 {
-    NPCMGMACState *s = opaque;
+    NPCMGMACState *gmac = opaque;
+    uint32_t prev;
+
+    trace_npcm_gmac_reg_write(DEVICE(gmac)->canonical_path, offset, v);
 
-    trace_npcm_gmac_reg_write(DEVICE(s)->canonical_path, offset, v);
     switch (offset) {
     /* Read only registers */
     case A_NPCM_GMAC_VERSION:
@@ -250,25 +301,44 @@  static void npcm_gmac_write(void *opaque, hwaddr offset,
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Write of read-only reg: offset: 0x%04" HWADDR_PRIx
                       ", value: 0x%04" PRIx64 "\n",
-                      DEVICE(s)->canonical_path, offset, v);
+                      DEVICE(gmac)->canonical_path, offset, v);
+        break;
+
+    case A_NPCM_GMAC_MAC_CONFIG:
+        prev = gmac->regs[offset / sizeof(uint32_t)];
+        gmac->regs[offset / sizeof(uint32_t)] = v;
+
+        /* If transmit is being enabled for first time, update desc addr */
+        if (~(prev & NPCM_GMAC_MAC_CONFIG_TX_EN) &
+             (v & NPCM_GMAC_MAC_CONFIG_TX_EN)) {
+            gmac->regs[R_NPCM_DMA_HOST_TX_DESC] =
+                gmac->regs[R_NPCM_DMA_TX_BASE_ADDR];
+        }
+
+        /* If receive is being enabled for first time, update desc addr */
+        if (~(prev & NPCM_GMAC_MAC_CONFIG_RX_EN) &
+             (v & NPCM_GMAC_MAC_CONFIG_RX_EN)) {
+            gmac->regs[R_NPCM_DMA_HOST_RX_DESC] =
+                gmac->regs[R_NPCM_DMA_RX_BASE_ADDR];
+        }
         break;
 
     case A_NPCM_GMAC_MII_ADDR:
-        npcm_gmac_mdio_access(s, v);
+        npcm_gmac_mdio_access(gmac, v);
         break;
 
     case A_NPCM_GMAC_MAC0_ADDR_HI:
-        s->regs[offset / sizeof(uint32_t)] = v;
-        s->conf.macaddr.a[0] = v >> 8;
-        s->conf.macaddr.a[1] = v >> 0;
+        gmac->regs[offset / sizeof(uint32_t)] = v;
+        gmac->conf.macaddr.a[0] = v >> 8;
+        gmac->conf.macaddr.a[1] = v >> 0;
         break;
 
     case A_NPCM_GMAC_MAC0_ADDR_LO:
-        s->regs[offset / sizeof(uint32_t)] = v;
-        s->conf.macaddr.a[2] = v >> 24;
-        s->conf.macaddr.a[3] = v >> 16;
-        s->conf.macaddr.a[4] = v >> 8;
-        s->conf.macaddr.a[5] = v >> 0;
+        gmac->regs[offset / sizeof(uint32_t)] = v;
+        gmac->conf.macaddr.a[2] = v >> 24;
+        gmac->conf.macaddr.a[3] = v >> 16;
+        gmac->conf.macaddr.a[4] = v >> 8;
+        gmac->conf.macaddr.a[5] = v >> 0;
         break;
 
     case A_NPCM_GMAC_MAC1_ADDR_HI:
@@ -277,33 +347,60 @@  static void npcm_gmac_write(void *opaque, hwaddr offset,
     case A_NPCM_GMAC_MAC2_ADDR_LO:
     case A_NPCM_GMAC_MAC3_ADDR_HI:
     case A_NPCM_GMAC_MAC3_ADDR_LO:
-        s->regs[offset / sizeof(uint32_t)] = v;
+        gmac->regs[offset / sizeof(uint32_t)] = v;
         qemu_log_mask(LOG_UNIMP,
                       "%s: Only MAC Address 0 is supported. This request "
-                      "is ignored.\n", DEVICE(s)->canonical_path);
+                      "is ignored.\n", DEVICE(gmac)->canonical_path);
         break;
 
     case A_NPCM_DMA_BUS_MODE:
-        s->regs[offset / sizeof(uint32_t)] = v;
+        gmac->regs[offset / sizeof(uint32_t)] = v;
         if (v & NPCM_DMA_BUS_MODE_SWR) {
-            npcm_gmac_soft_reset(s);
+            npcm_gmac_soft_reset(gmac);
+        }
+        break;
+
+    case A_NPCM_DMA_RCV_POLL_DEMAND:
+        /* We dont actually care about the value */
+        break;
+
+    case A_NPCM_DMA_STATUS:
+        /* Check that RO bits are not written to */
+        if (NPCM_DMA_STATUS_RO_MASK(v)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Write of read-only bits of reg: offset: 0x%04"
+                           HWADDR_PRIx ", value: 0x%04" PRIx64 "\n",
+                           DEVICE(gmac)->canonical_path, offset, v);
+        } else {
+            /* for W1c bits, implement W1C */
+            gmac->regs[offset / sizeof(uint32_t)] &=
+                ~NPCM_DMA_STATUS_W1C_MASK(v);
+            if (v & NPCM_DMA_STATUS_NIS_BITS) {
+                gmac->regs[offset / sizeof(uint32_t)] &= ~NPCM_DMA_STATUS_NIS;
+            }
+            if (v & NPCM_DMA_STATUS_AIS_BITS) {
+                gmac->regs[offset / sizeof(uint32_t)] &= ~NPCM_DMA_STATUS_AIS;
+            }
         }
         break;
 
     default:
-        s->regs[offset / sizeof(uint32_t)] = v;
+        gmac->regs[offset / sizeof(uint32_t)] = v;
         break;
     }
+
+    gmac_update_irq(gmac);
 }
 
 static void npcm_gmac_reset(DeviceState *dev)
 {
-    NPCMGMACState *s = NPCM_GMAC(dev);
+    NPCMGMACState *gmac = NPCM_GMAC(dev);
 
-    npcm_gmac_soft_reset(s);
-    memcpy(s->phy_regs[0], phy_reg_init, sizeof(phy_reg_init));
+    npcm_gmac_soft_reset(gmac);
+    memcpy(gmac->phy_regs[0], phy_reg_init, sizeof(phy_reg_init));
 
-    trace_npcm_gmac_reset(DEVICE(s)->canonical_path, s->phy_regs[0][MII_BMSR]);
+    trace_npcm_gmac_reset(DEVICE(gmac)->canonical_path,
+                          gmac->phy_regs[0][MII_BMSR]);
 }
 
 static NetClientInfo net_npcm_gmac_info = {
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 33514548b8..9588dc151b 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -473,6 +473,15 @@  npcm_gmac_reg_write(const char *name, uint64_t offset, uint32_t value) "%s: offs
 npcm_gmac_mdio_access(const char *name, uint8_t is_write, uint8_t pa, uint8_t gr, uint16_t val) "%s: is_write: %" PRIu8 " pa: %" PRIu8 " gr: %" PRIu8 " val: 0x%04" PRIx16
 npcm_gmac_reset(const char *name, uint16_t value) "%s: phy_regs[0][1]: 0x%04" PRIx16
 npcm_gmac_set_link(bool active) "Set link: active=%u"
+npcm_gmac_update_irq(const char *name, uint32_t status, uint32_t intr_en, int level) "%s: Status Reg: 0x%04" PRIX32 " Interrupt Enable Reg: 0x%04" PRIX32 " IRQ Set: %d"
+npcm_gmac_packet_desc_read(const char* name, uint32_t desc_addr) "%s: attempting to read descriptor @0x%04" PRIX32
+npcm_gmac_packet_receive(const char* name, uint32_t len) "%s: RX packet length: 0x%04" PRIX32
+npcm_gmac_packet_receiving_buffer(const char* name, uint32_t buf_len, uint32_t rx_buf_addr) "%s: Receiving into Buffer size: 0x%04" PRIX32 " at address 0x%04" PRIX32
+npcm_gmac_packet_received(const char* name, uint32_t len) "%s: Reception finished, packet left: 0x%04" PRIX32
+npcm_gmac_packet_transmit(const char* name, uint16_t len) "%s: TX transmission start, packed length 0x%04" PRIX16
+npcm_gmac_packet_sent(const char* name, uint16_t len) "%s: TX packet sent!, length: 0x%04" PRIX16
+npcm_gmac_debug_desc_data(const char* name, void* addr, uint32_t des0, uint32_t des1, uint32_t des2, uint32_t des3)"%s: Address: %p Descriptor 0: 0x%04" PRIX32 " Descriptor 1: 0x%04" PRIX32 "Descriptor 2: 0x%04" PRIX32 " Descriptor 3: 0x%04" PRIX32
+npcm_gmac_packet_tx_desc_data(const char* name, uint32_t tdes0, uint32_t tdes1) "%s: Tdes0: 0x%04" PRIX32 " Tdes1: 0x%04" PRIX32
 
 # npcm_pcs.c
 npcm_pcs_reg_read(const char *name, uint16_t indirect_access_baes, uint64_t offset, uint16_t value) "%s: IND: 0x%02" PRIx16 " offset: 0x%04" PRIx64 " value: 0x%04" PRIx16
diff --git a/include/hw/net/npcm_gmac.h b/include/hw/net/npcm_gmac.h
index a92a654278..e5729e83ea 100644
--- a/include/hw/net/npcm_gmac.h
+++ b/include/hw/net/npcm_gmac.h
@@ -37,8 +37,6 @@  struct NPCMGMACRxDesc {
 /* RDES2 and RDES3 are buffer address pointers */
 /* Owner: 0 = software, 1 = gmac */
 #define RX_DESC_RDES0_OWNER_MASK BIT(31)
-/* Owner*/
-#define RX_DESC_RDES0_OWNER_SHIFT 31
 /* Destination Address Filter Fail */
 #define RX_DESC_RDES0_DEST_ADDR_FILT_FAIL_MASK BIT(30)
 /* Frame length*/
diff --git a/tests/qtest/npcm_gmac-test.c b/tests/qtest/npcm_gmac-test.c
index 77a83c4c58..130a1599a8 100644
--- a/tests/qtest/npcm_gmac-test.c
+++ b/tests/qtest/npcm_gmac-test.c
@@ -154,7 +154,7 @@  static void test_init(gconstpointer test_data)
     CHECK_REG32(NPCM_GMAC_MII_DATA, 0);
     CHECK_REG32(NPCM_GMAC_FLOW_CTRL, 0);
     CHECK_REG32(NPCM_GMAC_VLAN_FLAG, 0);
-    CHECK_REG32(NPCM_GMAC_VERSION, 0x00001037);
+    CHECK_REG32(NPCM_GMAC_VERSION, 0x00001032);
     CHECK_REG32(NPCM_GMAC_WAKEUP_FILTER, 0);
     CHECK_REG32(NPCM_GMAC_PMT, 0);
     CHECK_REG32(NPCM_GMAC_LPI_CTRL, 0);