[PATCH v5 0/3] hw/net/imx_fec: improve the imx fec emulator

[PATCH v5 0/3] hw/net/imx_fec: improve the imx fec emulator

[Jean-Christophe Dubois]

This series of path makes various improvement to the i.MX FEC ethernet
emulator.
  
 * PATCH 1: Convert the Ethernet emulator debug output to trace event
 * PATCH 2: Allow Ethernet PHY to be at any position on the MDIO bus
 * PATCH 3: Improve the i.MX FEC related PHY emulator by using standard
            header symbols instead of hardcoded values.

Jean-Christophe Dubois (3):
  hw/net/imx_fec: Convert debug fprintf() to trace events
  hw/net/imx_fec: Allow phy not to be the first device on the mii bus.
  hw/net/imx_fec: improve PHY implementation.

 hw/net/imx_fec.c     | 197 +++++++++++++++++++++----------------------
 hw/net/trace-events  |  18 ++++
 include/hw/net/mii.h |   4 +
 3 files changed, 119 insertions(+), 100 deletions(-)

-- 
2.25.1

[PATCH v5 1/3] hw/net/imx_fec: Convert debug fprintf() to trace events

[Jean-Christophe Dubois]

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200530102707.195131-1-jcd@tribudubois.net>
[PMD: Fixed 32-bit format string using PRIx32/PRIx64]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Based-on: <20200530102707.195131-1-jcd@tribudubois.net>
---
 v2: fix coding style issues.
 v3: improve tracing code based on feedback
     * change some tracing function names
     * remove unnecessary cast
     * add register index in addition to name
 v4: fix 32-bit format string using PRIx32/PRIx64
 v5: Nothing

 hw/net/imx_fec.c    | 106 +++++++++++++++++++-------------------------
 hw/net/trace-events |  18 ++++++++
 2 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 7adcc9df654..eefedc252de 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -31,34 +31,11 @@
 #include "qemu/module.h"
 #include "net/checksum.h"
 #include "net/eth.h"
+#include "trace.h"
 
 /* For crc32 */
 #include <zlib.h>
 
-#ifndef DEBUG_IMX_FEC
-#define DEBUG_IMX_FEC 0
-#endif
-
-#define FEC_PRINTF(fmt, args...) \
-    do { \
-        if (DEBUG_IMX_FEC) { \
-            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_FEC, \
-                                             __func__, ##args); \
-        } \
-    } while (0)
-
-#ifndef DEBUG_IMX_PHY
-#define DEBUG_IMX_PHY 0
-#endif
-
-#define PHY_PRINTF(fmt, args...) \
-    do { \
-        if (DEBUG_IMX_PHY) { \
-            fprintf(stderr, "[%s.phy]%s: " fmt , TYPE_IMX_FEC, \
-                                                 __func__, ##args); \
-        } \
-    } while (0)
-
 #define IMX_MAX_DESC    1024
 
 static const char *imx_default_reg_name(IMXFECState *s, uint32_t index)
@@ -262,43 +239,45 @@ static void imx_eth_update(IMXFECState *s);
  * For now we don't handle any GPIO/interrupt line, so the OS will
  * have to poll for the PHY status.
  */
-static void phy_update_irq(IMXFECState *s)
+static void imx_phy_update_irq(IMXFECState *s)
 {
     imx_eth_update(s);
 }
 
-static void phy_update_link(IMXFECState *s)
+static void imx_phy_update_link(IMXFECState *s)
 {
     /* Autonegotiation status mirrors link status.  */
     if (qemu_get_queue(s->nic)->link_down) {
-        PHY_PRINTF("link is down\n");
+        trace_imx_phy_update_link("down");
         s->phy_status &= ~0x0024;
         s->phy_int |= PHY_INT_DOWN;
     } else {
-        PHY_PRINTF("link is up\n");
+        trace_imx_phy_update_link("up");
         s->phy_status |= 0x0024;
         s->phy_int |= PHY_INT_ENERGYON;
         s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
     }
-    phy_update_irq(s);
+    imx_phy_update_irq(s);
 }
 
 static void imx_eth_set_link(NetClientState *nc)
 {
-    phy_update_link(IMX_FEC(qemu_get_nic_opaque(nc)));
+    imx_phy_update_link(IMX_FEC(qemu_get_nic_opaque(nc)));
 }
 
-static void phy_reset(IMXFECState *s)
+static void imx_phy_reset(IMXFECState *s)
 {
+    trace_imx_phy_reset();
+
     s->phy_status = 0x7809;
     s->phy_control = 0x3000;
     s->phy_advertise = 0x01e1;
     s->phy_int_mask = 0;
     s->phy_int = 0;
-    phy_update_link(s);
+    imx_phy_update_link(s);
 }
 
-static uint32_t do_phy_read(IMXFECState *s, int reg)
+static uint32_t imx_phy_read(IMXFECState *s, int reg)
 {
     uint32_t val;
 
@@ -332,7 +311,7 @@ static uint32_t do_phy_read(IMXFECState *s, int reg)
     case 29:    /* Interrupt source.  */
         val = s->phy_int;
         s->phy_int = 0;
-        phy_update_irq(s);
+        imx_phy_update_irq(s);
         break;
     case 30:    /* Interrupt mask */
         val = s->phy_int_mask;
@@ -352,14 +331,14 @@ static uint32_t do_phy_read(IMXFECState *s, int reg)
         break;
     }
 
-    PHY_PRINTF("read 0x%04x @ %d\n", val, reg);
+    trace_imx_phy_read(val, reg);
 
     return val;
 }
 
-static void do_phy_write(IMXFECState *s, int reg, uint32_t val)
+static void imx_phy_write(IMXFECState *s, int reg, uint32_t val)
 {
-    PHY_PRINTF("write 0x%04x @ %d\n", val, reg);
+    trace_imx_phy_write(val, reg);
 
     if (reg > 31) {
         /* we only advertise one phy */
@@ -369,7 +348,7 @@ static void do_phy_write(IMXFECState *s, int reg, uint32_t val)
     switch (reg) {
     case 0:     /* Basic Control */
         if (val & 0x8000) {
-            phy_reset(s);
+            imx_phy_reset(s);
         } else {
             s->phy_control = val & 0x7980;
             /* Complete autonegotiation immediately.  */
@@ -383,7 +362,7 @@ static void do_phy_write(IMXFECState *s, int reg, uint32_t val)
         break;
     case 30:    /* Interrupt mask */
         s->phy_int_mask = val & 0xff;
-        phy_update_irq(s);
+        imx_phy_update_irq(s);
         break;
     case 17:
     case 18:
@@ -402,6 +381,8 @@ static void do_phy_write(IMXFECState *s, int reg, uint32_t val)
 static void imx_fec_read_bd(IMXFECBufDesc *bd, dma_addr_t addr)
 {
     dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd));
+
+    trace_imx_fec_read_bd(addr, bd->flags, bd->length, bd->data);
 }
 
 static void imx_fec_write_bd(IMXFECBufDesc *bd, dma_addr_t addr)
@@ -412,6 +393,9 @@ static void imx_fec_write_bd(IMXFECBufDesc *bd, dma_addr_t addr)
 static void imx_enet_read_bd(IMXENETBufDesc *bd, dma_addr_t addr)
 {
     dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd));
+
+    trace_imx_enet_read_bd(addr, bd->flags, bd->length, bd->data,
+                   bd->option, bd->status);
 }
 
 static void imx_enet_write_bd(IMXENETBufDesc *bd, dma_addr_t addr)
@@ -471,11 +455,11 @@ static void imx_fec_do_tx(IMXFECState *s)
         int len;
 
         imx_fec_read_bd(&bd, addr);
-        FEC_PRINTF("tx_bd %x flags %04x len %d data %08x\n",
-                   addr, bd.flags, bd.length, bd.data);
         if ((bd.flags & ENET_BD_R) == 0) {
+
             /* Run out of descriptors to transmit.  */
-            FEC_PRINTF("tx_bd ran out of descriptors to transmit\n");
+            trace_imx_eth_tx_bd_busy();
+
             break;
         }
         len = bd.length;
@@ -552,11 +536,11 @@ static void imx_enet_do_tx(IMXFECState *s, uint32_t index)
         int len;
 
         imx_enet_read_bd(&bd, addr);
-        FEC_PRINTF("tx_bd %x flags %04x len %d data %08x option %04x "
-                   "status %04x\n", addr, bd.flags, bd.length, bd.data,
-                   bd.option, bd.status);
         if ((bd.flags & ENET_BD_R) == 0) {
             /* Run out of descriptors to transmit.  */
+
+            trace_imx_eth_tx_bd_busy();
+
             break;
         }
         len = bd.length;
@@ -633,7 +617,7 @@ static void imx_eth_enable_rx(IMXFECState *s, bool flush)
     s->regs[ENET_RDAR] = (bd.flags & ENET_BD_E) ? ENET_RDAR_RDAR : 0;
 
     if (!s->regs[ENET_RDAR]) {
-        FEC_PRINTF("RX buffer full\n");
+        trace_imx_eth_rx_bd_full();
     } else if (flush) {
         qemu_flush_queued_packets(qemu_get_queue(s->nic));
     }
@@ -676,7 +660,7 @@ static void imx_eth_reset(DeviceState *d)
     memset(s->tx_descriptor, 0, sizeof(s->tx_descriptor));
 
     /* We also reset the PHY */
-    phy_reset(s);
+    imx_phy_reset(s);
 }
 
 static uint32_t imx_default_read(IMXFECState *s, uint32_t index)
@@ -774,8 +758,7 @@ static uint64_t imx_eth_read(void *opaque, hwaddr offset, unsigned size)
         break;
     }
 
-    FEC_PRINTF("reg[%s] => 0x%" PRIx32 "\n", imx_eth_reg_name(s, index),
-                                              value);
+    trace_imx_eth_read(index, imx_eth_reg_name(s, index), value);
 
     return value;
 }
@@ -884,8 +867,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
     const bool single_tx_ring = !imx_eth_is_multi_tx_ring(s);
     uint32_t index = offset >> 2;
 
-    FEC_PRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx_eth_reg_name(s, index),
-                (uint32_t)value);
+    trace_imx_eth_write(index, imx_eth_reg_name(s, index), value);
 
     switch (index) {
     case ENET_EIR:
@@ -940,12 +922,12 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
         if (extract32(value, 29, 1)) {
             /* This is a read operation */
             s->regs[ENET_MMFR] = deposit32(s->regs[ENET_MMFR], 0, 16,
-                                           do_phy_read(s,
+                                           imx_phy_read(s,
                                                        extract32(value,
                                                                  18, 10)));
         } else {
             /* This a write operation */
-            do_phy_write(s, extract32(value, 18, 10), extract32(value, 0, 16));
+            imx_phy_write(s, extract32(value, 18, 10), extract32(value, 0, 16));
         }
         /* raise the interrupt as the PHY operation is done */
         s->regs[ENET_EIR] |= ENET_INT_MII;
@@ -1053,8 +1035,6 @@ static bool imx_eth_can_receive(NetClientState *nc)
 {
     IMXFECState *s = IMX_FEC(qemu_get_nic_opaque(nc));
 
-    FEC_PRINTF("\n");
-
     return !!s->regs[ENET_RDAR];
 }
 
@@ -1071,7 +1051,7 @@ static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
     unsigned int buf_len;
     size_t size = len;
 
-    FEC_PRINTF("len %d\n", (int)size);
+    trace_imx_fec_receive(size);
 
     if (!s->regs[ENET_RDAR]) {
         qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Unexpected packet\n",
@@ -1113,7 +1093,7 @@ static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
         bd.length = buf_len;
         size -= buf_len;
 
-        FEC_PRINTF("rx_bd 0x%x length %d\n", addr, bd.length);
+        trace_imx_fec_receive_len(addr, bd.length);
 
         /* The last 4 bytes are the CRC.  */
         if (size < 4) {
@@ -1131,7 +1111,9 @@ static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
         if (size == 0) {
             /* Last buffer in frame.  */
             bd.flags |= flags | ENET_BD_L;
-            FEC_PRINTF("rx frame flags %04x\n", bd.flags);
+
+            trace_imx_fec_receive_last(bd.flags);
+
             s->regs[ENET_EIR] |= ENET_INT_RXF;
         } else {
             s->regs[ENET_EIR] |= ENET_INT_RXB;
@@ -1164,7 +1146,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
     size_t size = len;
     bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
 
-    FEC_PRINTF("len %d\n", (int)size);
+    trace_imx_enet_receive(size);
 
     if (!s->regs[ENET_RDAR]) {
         qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Unexpected packet\n",
@@ -1210,7 +1192,7 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
         bd.length = buf_len;
         size -= buf_len;
 
-        FEC_PRINTF("rx_bd 0x%x length %d\n", addr, bd.length);
+        trace_imx_enet_receive_len(addr, bd.length);
 
         /* The last 4 bytes are the CRC.  */
         if (size < 4) {
@@ -1246,7 +1228,9 @@ static ssize_t imx_enet_receive(NetClientState *nc, const uint8_t *buf,
         if (size == 0) {
             /* Last buffer in frame.  */
             bd.flags |= flags | ENET_BD_L;
-            FEC_PRINTF("rx frame flags %04x\n", bd.flags);
+
+            trace_imx_enet_receive_last(bd.flags);
+
             /* Indicate that we've updated the last buffer descriptor. */
             bd.last_buffer = ENET_BD_BDU;
             if (bd.option & ENET_BD_RX_INT) {
diff --git a/hw/net/trace-events b/hw/net/trace-events
index e18f883cfd4..26700dad997 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -408,3 +408,21 @@ i82596_receive_packet(size_t sz) "len=%zu"
 i82596_new_mac(const char *id_with_mac) "New MAC for: %s"
 i82596_set_multicast(uint16_t count) "Added %d multicast entries"
 i82596_channel_attention(void *s) "%p: Received CHANNEL ATTENTION"
+
+# imx_fec.c
+imx_phy_read(uint32_t val, int reg) "0x%04"PRIx32" <= reg[%d]"
+imx_phy_write(uint32_t val, int reg) "0x%04"PRIx32" => reg[%d]"
+imx_phy_update_link(const char *s) "%s"
+imx_phy_reset(void) ""
+imx_fec_read_bd(uint64_t addr, int flags, int len, int data) "tx_bd 0x%"PRIx64" flags 0x%04x len %d data 0x%08x"
+imx_enet_read_bd(uint64_t addr, int flags, int len, int data, int options, int status) "tx_bd 0x%"PRIx64" flags 0x%04x len %d data 0x%08x option 0x%04x status 0x%04x"
+imx_eth_tx_bd_busy(void) "tx_bd ran out of descriptors to transmit"
+imx_eth_rx_bd_full(void) "RX buffer is full"
+imx_eth_read(int reg, const char *reg_name, uint32_t value) "reg[%d:%s] => 0x%08"PRIx32
+imx_eth_write(int reg, const char *reg_name, uint64_t value) "reg[%d:%s] <= 0x%08"PRIx64
+imx_fec_receive(size_t size) "len %zu"
+imx_fec_receive_len(uint64_t addr, int len) "rx_bd 0x%"PRIx64" length %d"
+imx_fec_receive_last(int last) "rx frame flags 0x%04x"
+imx_enet_receive(size_t size) "len %zu"
+imx_enet_receive_len(uint64_t addr, int len) "rx_bd 0x%"PRIx64" length %d"
+imx_enet_receive_last(int last) "rx frame flags 0x%04x"
-- 
2.25.1

[PATCH v5 2/3] hw/net/imx_fec: Allow phy not to be the first device on the mii bus.

[Jean-Christophe Dubois]

Up to now we were allowing only one PHY device and it had to be the
first device on the bus.

The i.MX6UL has 2 Ethernet devices and can therefore have several
PHY devices on the bus (and not necessarilly as device 0).

This patch allows for PHY devices on 2nd, 3rd or any position.

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
 v2: Not present
 v3: Not present
 v4: Not present
 v5: Allow phy not to be the first device on the mii bus.

 hw/net/imx_fec.c    | 19 ++++++++-----------
 hw/net/trace-events |  4 ++--
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index eefedc252de..29e613699ee 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s)
 static uint32_t imx_phy_read(IMXFECState *s, int reg)
 {
     uint32_t val;
+    uint32_t phy = reg / 32;
 
-    if (reg > 31) {
-        /* we only advertise one phy */
-        return 0;
-    }
+    reg %= 32;
 
     switch (reg) {
     case 0:     /* Basic Control */
@@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
         break;
     }
 
-    trace_imx_phy_read(val, reg);
+    trace_imx_phy_read(val, phy, reg);
 
     return val;
 }
 
 static void imx_phy_write(IMXFECState *s, int reg, uint32_t val)
 {
-    trace_imx_phy_write(val, reg);
+    uint32_t phy = reg / 32;
 
-    if (reg > 31) {
-        /* we only advertise one phy */
-        return;
-    }
+    reg %= 32;
+
+    trace_imx_phy_write(val, phy, reg);
 
     switch (reg) {
     case 0:     /* Basic Control */
@@ -926,7 +923,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
                                                        extract32(value,
                                                                  18, 10)));
         } else {
-            /* This a write operation */
+            /* This is a write operation */
             imx_phy_write(s, extract32(value, 18, 10), extract32(value, 0, 16));
         }
         /* raise the interrupt as the PHY operation is done */
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 26700dad997..27dfa0ef775 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -410,8 +410,8 @@ i82596_set_multicast(uint16_t count) "Added %d multicast entries"
 i82596_channel_attention(void *s) "%p: Received CHANNEL ATTENTION"
 
 # imx_fec.c
-imx_phy_read(uint32_t val, int reg) "0x%04"PRIx32" <= reg[%d]"
-imx_phy_write(uint32_t val, int reg) "0x%04"PRIx32" => reg[%d]"
+imx_phy_read(uint32_t val, int phy, int reg) "0x%04"PRIx32" <= phy[%d].reg[%d]"
+imx_phy_write(uint32_t val, int phy, int reg) "0x%04"PRIx32" => phy[%d].reg[%d]"
 imx_phy_update_link(const char *s) "%s"
 imx_phy_reset(void) ""
 imx_fec_read_bd(uint64_t addr, int flags, int len, int data) "tx_bd 0x%"PRIx64" flags 0x%04x len %d data 0x%08x"
-- 
2.25.1

[PATCH v5 3/3] hw/net/imx_fec: improve PHY implementation.

[Jean-Christophe Dubois]

improve the PHY implementation with more generic code.

This patch remove a lot of harcoded values to replace them with
generic symbols from header files.

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
 v2: Not present
 v3: Not present
 v4: Not present
 v5: improve PHY implementation.

 hw/net/imx_fec.c     | 76 +++++++++++++++++++++++++++-----------------
 include/hw/net/mii.h |  4 +++
 2 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index 29e613699ee..bf9583a93f4 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -24,6 +24,7 @@
 #include "qemu/osdep.h"
 #include "hw/irq.h"
 #include "hw/net/imx_fec.h"
+#include "hw/net/mii.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "sysemu/dma.h"
@@ -231,6 +232,9 @@ static const VMStateDescription vmstate_imx_eth = {
 #define PHY_INT_PARFAULT            (1 << 2)
 #define PHY_INT_AUTONEG_PAGE        (1 << 1)
 
+#define MII_SMC911X_ISF             29
+#define MII_SMC911X_IM              30
+
 static void imx_eth_update(IMXFECState *s);
 
 /*
@@ -249,11 +253,11 @@ static void imx_phy_update_link(IMXFECState *s)
     /* Autonegotiation status mirrors link status.  */
     if (qemu_get_queue(s->nic)->link_down) {
         trace_imx_phy_update_link("down");
-        s->phy_status &= ~0x0024;
+        s->phy_status &= ~(MII_BMSR_LINK_ST | MII_BMSR_AN_COMP);
         s->phy_int |= PHY_INT_DOWN;
     } else {
         trace_imx_phy_update_link("up");
-        s->phy_status |= 0x0024;
+        s->phy_status |= MII_BMSR_LINK_ST | MII_BMSR_AN_COMP;
         s->phy_int |= PHY_INT_ENERGYON;
         s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
     }
@@ -269,9 +273,11 @@ static void imx_phy_reset(IMXFECState *s)
 {
     trace_imx_phy_reset();
 
-    s->phy_status = 0x7809;
-    s->phy_control = 0x3000;
-    s->phy_advertise = 0x01e1;
+    s->phy_status = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
+                    MII_BMSR_10T_HD | MII_BMSR_AUTONEG | MII_BMSR_EXTCAP;
+    s->phy_control = MII_BMCR_AUTOEN | MII_BMCR_SPEED100;
+    s->phy_advertise = MII_ANAR_CSMACD | MII_ANAR_TX | MII_ANAR_10FD |
+                       MII_ANAR_10 | MII_ANAR_TXFD;
     s->phy_int_mask = 0;
     s->phy_int = 0;
     imx_phy_update_link(s);
@@ -285,37 +291,42 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
     reg %= 32;
 
     switch (reg) {
-    case 0:     /* Basic Control */
+    case MII_BMCR:     /* Basic Control */
         val = s->phy_control;
         break;
-    case 1:     /* Basic Status */
+    case MII_BMSR:     /* Basic Status */
         val = s->phy_status;
         break;
-    case 2:     /* ID1 */
-        val = 0x0007;
+    case MII_PHYID1:     /* ID1 */
+        val = LAN911x_PHYID1;
         break;
-    case 3:     /* ID2 */
-        val = 0xc0d1;
+    case MII_PHYID2:     /* ID2 */
+        val = LAN911x_PHYID2;
         break;
-    case 4:     /* Auto-neg advertisement */
+    case MII_ANAR:     /* Auto-neg advertisement */
         val = s->phy_advertise;
         break;
-    case 5:     /* Auto-neg Link Partner Ability */
-        val = 0x0f71;
+    case MII_ANLPAR:     /* Auto-neg Link Partner Ability */
+        val = MII_ANLPAR_CSMACD | MII_ANLPAR_10 | MII_ANLPAR_10FD |
+              MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE |
+              MII_ANLPAR_PAUSEASY;
         break;
-    case 6:     /* Auto-neg Expansion */
-        val = 1;
+    case MII_ANER:     /* Auto-neg Expansion */
+        val = MII_ANER_NWAY;
         break;
-    case 29:    /* Interrupt source.  */
+    case MII_SMC911X_ISF:    /* Interrupt source.  */
         val = s->phy_int;
         s->phy_int = 0;
         imx_phy_update_irq(s);
         break;
-    case 30:    /* Interrupt mask */
+    case MII_SMC911X_IM:    /* Interrupt mask */
         val = s->phy_int_mask;
         break;
-    case 17:
-    case 18:
+    case MII_NSR:
+        val = 1 << 6;
+        break;
+    case MII_LBREMR:
+    case MII_REC:
     case 27:
     case 31:
         qemu_log_mask(LOG_UNIMP, "[%s.phy]%s: reg %d not implemented\n",
@@ -343,26 +354,31 @@ static void imx_phy_write(IMXFECState *s, int reg, uint32_t val)
     trace_imx_phy_write(val, phy, reg);
 
     switch (reg) {
-    case 0:     /* Basic Control */
-        if (val & 0x8000) {
+    case MII_BMCR:     /* Basic Control */
+        if (val & MII_BMCR_RESET) {
             imx_phy_reset(s);
         } else {
-            s->phy_control = val & 0x7980;
+            s->phy_control = val & (MII_BMCR_LOOPBACK | MII_BMCR_SPEED100 |
+                                    MII_BMCR_AUTOEN | MII_BMCR_PDOWN |
+                                    MII_BMCR_FD | MII_BMCR_CTST);
             /* Complete autonegotiation immediately.  */
-            if (val & 0x1000) {
-                s->phy_status |= 0x0020;
+            if (val & MII_BMCR_AUTOEN) {
+                s->phy_status |= MII_BMSR_AN_COMP;
             }
         }
         break;
-    case 4:     /* Auto-neg advertisement */
-        s->phy_advertise = (val & 0x2d7f) | 0x80;
+    case MII_ANAR:     /* Auto-neg advertisement */
+        s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE |
+                                   MII_ANAR_TXFD | MII_ANAR_TX |
+                                   MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) |
+                                   MII_ANAR_TX;
         break;
-    case 30:    /* Interrupt mask */
+    case MII_SMC911X_IM:    /* Interrupt mask */
         s->phy_int_mask = val & 0xff;
         imx_phy_update_irq(s);
         break;
-    case 17:
-    case 18:
+    case MII_LBREMR:
+    case MII_REC:
     case 27:
     case 31:
         qemu_log_mask(LOG_UNIMP, "[%s.phy)%s: reg %d not implemented\n",
diff --git a/include/hw/net/mii.h b/include/hw/net/mii.h
index 4ae4dcce7e3..d2001bd859b 100644
--- a/include/hw/net/mii.h
+++ b/include/hw/net/mii.h
@@ -112,4 +112,8 @@
 #define DP83848_PHYID1      0x2000
 #define DP83848_PHYID2      0x5c90
 
+/* SMSC LAN911x Internal PHY */
+#define LAN911x_PHYID1      0x0007
+#define LAN911x_PHYID2      0xc0d1
+
 #endif /* MII_H */
-- 
2.25.1

Re: [PATCH v5 2/3] hw/net/imx_fec: Allow phy not to be the first device on the mii bus.

[Peter Maydell]

On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>
> Up to now we were allowing only one PHY device and it had to be the
> first device on the bus.
>
> The i.MX6UL has 2 Ethernet devices and can therefore have several
> PHY devices on the bus (and not necessarilly as device 0).
>
> This patch allows for PHY devices on 2nd, 3rd or any position.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>

> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index eefedc252de..29e613699ee 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s)
>  static uint32_t imx_phy_read(IMXFECState *s, int reg)
>  {
>      uint32_t val;
> +    uint32_t phy = reg / 32;
>
> -    if (reg > 31) {
> -        /* we only advertise one phy */
> -        return 0;
> -    }
> +    reg %= 32;

This change means we now support multiple PHYs...

>
>      switch (reg) {
>      case 0:     /* Basic Control */
> @@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
>          break;
>      }
>
> -    trace_imx_phy_read(val, reg);
> +    trace_imx_phy_read(val, phy, reg);

...but the only change we actually make is to trace the phy number.
Surely if there is more than one phy then each phy needs to have
its own state (phy_control/phy_status/phy_advertise/etc), rather
than all these PHYs all sharing the same state under the hood?

It also sounds from your commit message as if there isn't a
large number of PHYs, but only perhaps two. In that case
don't we need to fail attempts to access non-existent PHYs
and only work with the ones which actually exist on any
given board?

thanks
-- PMM

Re: [PATCH v5 3/3] hw/net/imx_fec: improve PHY implementation.

[Peter Maydell]

On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>
> improve the PHY implementation with more generic code.
>
> This patch remove a lot of harcoded values to replace them with
> generic symbols from header files.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
>  v2: Not present
>  v3: Not present
>  v4: Not present
>  v5: improve PHY implementation.
>
>  hw/net/imx_fec.c     | 76 +++++++++++++++++++++++++++-----------------
>  include/hw/net/mii.h |  4 +++
>  2 files changed, 50 insertions(+), 30 deletions(-)


> -    case 5:     /* Auto-neg Link Partner Ability */
> -        val = 0x0f71;
> +    case MII_ANLPAR:     /* Auto-neg Link Partner Ability */
> +        val = MII_ANLPAR_CSMACD | MII_ANLPAR_10 | MII_ANLPAR_10FD |
> +              MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE |
> +              MII_ANLPAR_PAUSEASY;

The old value is 0x0f71, but the new one with the constants
is 0x0de1.


> -    case 30:    /* Interrupt mask */
> +    case MII_SMC911X_IM:    /* Interrupt mask */
>          val = s->phy_int_mask;
>          break;
> -    case 17:
> -    case 18:
> +    case MII_NSR:
> +        val = 1 << 6;
> +        break;

The old code didn't have a case for MII_NSR (16).

> +    case MII_LBREMR:
> +    case MII_REC:
>      case 27:
>      case 31:


> -    case 4:     /* Auto-neg advertisement */
> -        s->phy_advertise = (val & 0x2d7f) | 0x80;
> +    case MII_ANAR:     /* Auto-neg advertisement */
> +        s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE |
> +                                   MII_ANAR_TXFD | MII_ANAR_TX |
> +                                   MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) |
> +                                   MII_ANAR_TX;

The old code does & 0x2d7f; the new code is & 0xdff.

>          break;

If some of these are bug fixes, please can you put them in a separate
patch, so that the "use symbolic constants" change can be reviewed
as making no functional changes?

thanks
-- PMM

Re: [PATCH v5 0/3] hw/net/imx_fec: improve the imx fec emulator

[Peter Maydell]

On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>
> This series of path makes various improvement to the i.MX FEC ethernet
> emulator.
>
>  * PATCH 1: Convert the Ethernet emulator debug output to trace event
>  * PATCH 2: Allow Ethernet PHY to be at any position on the MDIO bus
>  * PATCH 3: Improve the i.MX FEC related PHY emulator by using standard
>             header symbols instead of hardcoded values.
>
> Jean-Christophe Dubois (3):
>   hw/net/imx_fec: Convert debug fprintf() to trace events
>   hw/net/imx_fec: Allow phy not to be the first device on the mii bus.
>   hw/net/imx_fec: improve PHY implementation.

Hi; I've applied patch 1 to target-arm.next and left review comments
for patches 2 and 3.

thanks
-- PMM

Re: [PATCH v5 2/3] hw/net/imx_fec: Allow phy not to be the first device on the mii bus.

[Jean-Christophe DUBOIS]

Le 15/06/2020 à 14:23, Peter Maydell a écrit :
> On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> Up to now we were allowing only one PHY device and it had to be the
>> first device on the bus.
>>
>> The i.MX6UL has 2 Ethernet devices and can therefore have several
>> PHY devices on the bus (and not necessarilly as device 0).
>>
>> This patch allows for PHY devices on 2nd, 3rd or any position.
>>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index eefedc252de..29e613699ee 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s)
>>   static uint32_t imx_phy_read(IMXFECState *s, int reg)
>>   {
>>       uint32_t val;
>> +    uint32_t phy = reg / 32;
>>
>> -    if (reg > 31) {
>> -        /* we only advertise one phy */
>> -        return 0;
>> -    }
>> +    reg %= 32;
> This change means we now support multiple PHYs...

Yes, on the i.MX6UL there are 2 ENET devices but only one MDIO bus to 
acess the PHYs.

On the i.MX6UL evaluation board, PHY seems to be at offset/adress 1 and 2.

See linux/arch/arm/boot/dts/imx6ul-14x14-evk.dtsi for details.

>
>>       switch (reg) {
>>       case 0:     /* Basic Control */
>> @@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
>>           break;
>>       }
>>
>> -    trace_imx_phy_read(val, reg);
>> +    trace_imx_phy_read(val, phy, reg);
> ...but the only change we actually make is to trace the phy number.
> Surely if there is more than one phy then each phy needs to have
> its own state (phy_control/phy_status/phy_advertise/etc), rather
> than all these PHYs all sharing the same state under the hood?

Well there might be several PHYs on the MDIO bus but each PHY is used 
only by one ENET device. There is no plan for one ENET to use either 
PHY. It can only use one.

In Qemu (or at least in the FEC ethernet device emulator) the phy state 
is embedded in the ethernet device state.

>
> It also sounds from your commit message as if there isn't a
> large number of PHYs, but only perhaps two. In that case
> don't we need to fail attempts to access non-existent PHYs
> and only work with the ones which actually exist on any
> given board?

As stated on the particular i.MX6UL evaluation board there are 2 phys 
connected to the MDIO bus at adresse 1 and 2.

On another board the PHYs could be at different addresses or we might 
use only one MAC and therefore only one PHY.

So in order to be able to fail on bad PHY access we need to discriminate 
for each board which PHYs are actually present and at what address on 
the MDIO bus. We would also need to know which PHY is connected to which 
MAC.

I might have overlook something but so far there is no clear separate 
PHY and MAC implementation.

Usually the PHY is more or less part of the MAC implementation and there 
are no easy way to instantiate them separately then connect them with 
the required bus (MDIO and MAC).

I guess it might be feasible even if it sounds like overkill most of the 
time (you usually get one MAC connected to one PHY).

Is there such a qemu framework that would allow to connect multiple PHY 
to a single MDIO bus and then each PHY to a specific MAC device? Could 
you point me in the right direction ?

>
> thanks
> -- PMM

Re: [PATCH v5 3/3] hw/net/imx_fec: improve PHY implementation.

[Jean-Christophe DUBOIS]

Le 15/06/2020 à 15:03, Peter Maydell a écrit :
> On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> improve the PHY implementation with more generic code.
>>
>> This patch remove a lot of harcoded values to replace them with
>> generic symbols from header files.
>>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> ---
>>   v2: Not present
>>   v3: Not present
>>   v4: Not present
>>   v5: improve PHY implementation.
>>
>>   hw/net/imx_fec.c     | 76 +++++++++++++++++++++++++++-----------------
>>   include/hw/net/mii.h |  4 +++
>>   2 files changed, 50 insertions(+), 30 deletions(-)
>
>> -    case 5:     /* Auto-neg Link Partner Ability */
>> -        val = 0x0f71;
>> +    case MII_ANLPAR:     /* Auto-neg Link Partner Ability */
>> +        val = / | MII_ANLPAR_10 | MII_ANLPAR_10FD |
>> +              MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE |
>> +              MII_ANLPAR_PAUSEASY;
> The old value is 0x0f71, but the new one with the constants
> is 0x0de1.

First of I should say that this PHY, first borrowed by the mfc_fec.c 
(coldfire ethernet device) from lan9118 (and now by imx_fec.c) is not 
one used on any real i.MX (i.MX6, i.MX7, i.MX31, i.MX25, ...) based 
board that I know of (this particular PHY is embedded n the lan9118 
ethernet device)

It is there because we were in need of a PHY and this PHY needs to be 
simple and more or less standard.

I might have missed something but I am not really aware of way in Qemu 
to swap PHYs for a given ethernet emulator depending on the emulated board.

So here this PHY was just a blind cut and paste of the lan9118.c PHY 
part to get a reasonable working PHY for the FEC/ENET device.

So here the previous value of this register is not really meaningful. It 
is a mix of standard MII defined bits and LAN911X specific bits (for 
which I don't necessarily have definition ).

Here I decided to restrict the implementation of this rather "virtual" 
PHY to only standard defined bits

actually I think, I should have removed a lot more lan911x specific 
bits/registers to get to a really simple/trivial standard PHY.

>> -    case 30:    /* Interrupt mask */
>> +    case MII_SMC911X_IM:    /* Interrupt mask */
>>           val = s->phy_int_mask;
>>           break;
>> -    case 17:
>> -    case 18:
>> +    case MII_NSR:
>> +        val = 1 << 6;
>> +        break;
> The old code didn't have a case for MII_NSR (16).

I am not sure anymore why I added MII_NSR register. It is not present on 
lan9118 ethernet device but it is a standard defined register.

>> +    case MII_LBREMR:
>> +    case MII_REC:
>>       case 27:
>>       case 31:
>
>> -    case 4:     /* Auto-neg advertisement */
>> -        s->phy_advertise = (val & 0x2d7f) | 0x80;
>> +    case MII_ANAR:     /* Auto-neg advertisement */
>> +        s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE |
>> +                                   MII_ANAR_TXFD | MII_ANAR_TX |
>> +                                   MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) |
>> +                                   MII_ANAR_TX;
> The old code does & 0x2d7f; the new code is & 0xdff.
Same reason as the ANLPAR register.
>>           break;
> If some of these are bug fixes, please can you put them in a separate
> patch, so that the "use symbolic constants" change can be reviewed
> as making no functional changes?
>
> thanks
> -- PMM
>