[PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes

[PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes

[Doug Brown]

This series fixes several problems I ran into while trying to simulate
the AMD/Xilinx Versal CANFD controller in the xlnx-versal-virt machine
using Xilinx's v6.6_LTS_2024.1 kernel. With all of these patches
applied, everything works correctly alongside actual CAN devices.

- IRQs were accidentally not being delivered due to having a level other
  than 1. The IRQ count in /proc/interrupts in Linux was stuck at 0.
- Incoming CAN FD frames were being treated as non-FD.
- The CAN IDs were garbled in both RX and TX directions.
- The ESI and BRS flags were not being handled.
- The byte ordering was wrong in the data in both directions.
- Incoming CAN FD frames with DLC = 1-7 weren't handled correctly.
- The FIFO read_index and store_index wrapping logic was incorrect.

I don't have any actual Versal hardware to compare behavior against, but
with these changes, it plays nicely with SocketCAN on the host system.

Changes in v2:
- Added handling of ESI and BRS flags, ensured frame->flags is initialized
- Switched to use common can_dlc2len() and can_len2dlc() functions
- Added fix for FIFO wrapping problems I observed during stress testing

Doug Brown (7):
  hw/net/can/xlnx-versal-canfd: Fix interrupt level
  hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check
  hw/net/can/xlnx-versal-canfd: Translate CAN ID registers
  hw/net/can/xlnx-versal-canfd: Handle flags correctly
  hw/net/can/xlnx-versal-canfd: Fix byte ordering
  hw/net/can/xlnx-versal-canfd: Simplify DLC conversions
  hw/net/can/xlnx-versal-canfd: Fix FIFO issues

 hw/net/can/xlnx-versal-canfd.c | 173 ++++++++++++++-------------------
 1 file changed, 72 insertions(+), 101 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] hw/net/can/xlnx-versal-canfd: Fix interrupt level

[Doug Brown]

The interrupt level should be 0 or 1. The existing code was using the
interrupt flags to determine the level. In the only machine currently
supported (xlnx-versal-virt), the GICv3 was masking off all bits except
bit 0 when applying it, resulting in the IRQ never being delivered.

Signed-off-by: Doug Brown <doug@schmorgal.com>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
---
 hw/net/can/xlnx-versal-canfd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 5f083c21e9..ad0c4da3c8 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -682,8 +682,8 @@ static uint8_t canfd_dlc_array[8] = {8, 12, 16, 20, 24, 32, 48, 64};
 
 static void canfd_update_irq(XlnxVersalCANFDState *s)
 {
-    unsigned int irq = s->regs[R_INTERRUPT_STATUS_REGISTER] &
-                        s->regs[R_INTERRUPT_ENABLE_REGISTER];
+    const bool irq = (s->regs[R_INTERRUPT_STATUS_REGISTER] &
+                      s->regs[R_INTERRUPT_ENABLE_REGISTER]) != 0;
     g_autofree char *path = object_get_canonical_path(OBJECT(s));
 
     /* RX watermark interrupts. */
-- 
2.34.1

[PATCH v2 2/7] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check

[Doug Brown]

When checking the QEMU_CAN_FRMF_TYPE_FD flag, we need to ignore other
potentially set flags. Before this change, received CAN FD frames from
SocketCAN weren't being recognized as CAN FD.

Signed-off-by: Doug Brown <doug@schmorgal.com>
Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
---
 hw/net/can/xlnx-versal-canfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index ad0c4da3c8..8968672b84 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -1003,7 +1003,7 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
 
         dlc = frame->can_dlc;
 
-        if (frame->flags == QEMU_CAN_FRMF_TYPE_FD) {
+        if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
             is_canfd_frame = true;
 
             /* Store dlc value in Xilinx specific format. */
-- 
2.34.1

[PATCH v2 3/7] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers

[Doug Brown]

Previously the emulated CAN ID register was being set to the exact same
value stored in qemu_can_frame.can_id. This doesn't work correctly
because the Xilinx IP core uses a different bit arrangement than
qemu_can_frame for all of its ID registers. Correct this problem for
both RX and TX, including RX filtering.

Signed-off-by: Doug Brown <doug@schmorgal.com>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
---
 hw/net/can/xlnx-versal-canfd.c | 53 ++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 8968672b84..1704b558d0 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -869,6 +869,8 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
     uint32_t val = 0;
     uint32_t dlc_reg_val = 0;
     uint32_t dlc_value = 0;
+    uint32_t id_reg_val = 0;
+    bool is_rtr = false;
 
     /* Check that reg_num should be within TX register space. */
     assert(reg_num <= R_TB_ID_REGISTER + (NUM_REGS_PER_MSG_SPACE *
@@ -877,7 +879,20 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
     dlc_reg_val = s->regs[reg_num + 1];
     dlc_value = FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, DLC);
 
-    frame->can_id = s->regs[reg_num];
+    id_reg_val = s->regs[reg_num];
+    if (FIELD_EX32(id_reg_val, TB_ID_REGISTER, IDE)) {
+        frame->can_id = (FIELD_EX32(id_reg_val, TB_ID_REGISTER, ID) << 18) |
+                        (FIELD_EX32(id_reg_val, TB_ID_REGISTER, ID_EXT)) |
+                        QEMU_CAN_EFF_FLAG;
+        if (FIELD_EX32(id_reg_val, TB_ID_REGISTER, RTR_RRS)) {
+            is_rtr = true;
+        }
+    } else {
+        frame->can_id = FIELD_EX32(id_reg_val, TB_ID_REGISTER, ID);
+        if (FIELD_EX32(id_reg_val, TB_ID_REGISTER, SRR_RTR_RRS)) {
+            is_rtr = true;
+        }
+    }
 
     if (FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, FDF)) {
         /*
@@ -923,6 +938,10 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
         } else {
             frame->can_dlc = dlc_value;
         }
+
+        if (is_rtr) {
+            frame->can_id |= QEMU_CAN_RTR_FLAG;
+        }
     }
 
     for (j = 0; j < frame->can_dlc; j++) {
@@ -948,6 +967,33 @@ static void process_cancellation_requests(XlnxVersalCANFDState *s)
     canfd_update_irq(s);
 }
 
+static uint32_t frame_to_reg_id(const qemu_can_frame *frame)
+{
+    uint32_t id_reg_val = 0;
+    const bool is_canfd_frame = frame->flags & QEMU_CAN_FRMF_TYPE_FD;
+    const bool is_rtr = !is_canfd_frame && (frame->can_id & QEMU_CAN_RTR_FLAG);
+
+    if (frame->can_id & QEMU_CAN_EFF_FLAG) {
+        id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, ID,
+                                 (frame->can_id & QEMU_CAN_EFF_MASK) >> 18);
+        id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, ID_EXT,
+                                 frame->can_id & QEMU_CAN_EFF_MASK);
+        id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, IDE, 1);
+        id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, SRR_RTR_RRS, 1);
+        if (is_rtr) {
+            id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, RTR_RRS, 1);
+        }
+    } else {
+        id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, ID,
+                                 frame->can_id & QEMU_CAN_SFF_MASK);
+        if (is_rtr) {
+            id_reg_val |= FIELD_DP32(0, RB_ID_REGISTER, SRR_RTR_RRS, 1);
+        }
+    }
+
+    return id_reg_val;
+}
+
 static void store_rx_sequential(XlnxVersalCANFDState *s,
                                 const qemu_can_frame *frame,
                                 uint32_t fill_level, uint32_t read_index,
@@ -999,7 +1045,7 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
                                                     NUM_REGS_PER_MSG_SPACE));
         }
 
-        s->regs[store_location] = frame->can_id;
+        s->regs[store_location] = frame_to_reg_id(frame);
 
         dlc = frame->can_dlc;
 
@@ -1090,11 +1136,12 @@ static void update_rx_sequential(XlnxVersalCANFDState *s,
     if (s->regs[R_ACCEPTANCE_FILTER_CONTROL_REGISTER]) {
         uint32_t acceptance_filter_status =
                                 s->regs[R_ACCEPTANCE_FILTER_CONTROL_REGISTER];
+        const uint32_t reg_id = frame_to_reg_id(frame);
 
         for (i = 0; i < 32; i++) {
             if (acceptance_filter_status & 0x1) {
                 uint32_t msg_id_masked = s->regs[R_AFMR_REGISTER + 2 * i] &
-                                         frame->can_id;
+                                         reg_id;
                 uint32_t afir_id_masked = s->regs[R_AFIR_REGISTER + 2 * i] &
                                           s->regs[R_AFMR_REGISTER + 2 * i];
                 uint16_t std_msg_id_masked = FIELD_EX32(msg_id_masked,
-- 
2.34.1

[PATCH v2 4/7] hw/net/can/xlnx-versal-canfd: Handle flags correctly

[Doug Brown]

Add support for QEMU_CAN_FRMF_ESI and QEMU_CAN_FRMF_BRS flags, and
ensure frame->flags is always initialized to 0.

Note that the Xilinx IP core doesn't allow manually setting the ESI bit
during transmits, so it's only implemented for the receive case.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 hw/net/can/xlnx-versal-canfd.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 1704b558d0..47631917ab 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -872,6 +872,8 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
     uint32_t id_reg_val = 0;
     bool is_rtr = false;
 
+    frame->flags = 0;
+
     /* Check that reg_num should be within TX register space. */
     assert(reg_num <= R_TB_ID_REGISTER + (NUM_REGS_PER_MSG_SPACE *
                                           s->cfg.tx_fifo));
@@ -913,7 +915,7 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
          *  15                         49 - 64
          */
 
-        frame->flags = QEMU_CAN_FRMF_TYPE_FD;
+        frame->flags |= QEMU_CAN_FRMF_TYPE_FD;
 
         if (dlc_value < 8) {
             frame->can_dlc = dlc_value;
@@ -921,6 +923,10 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
             assert((dlc_value - 8) < ARRAY_SIZE(canfd_dlc_array));
             frame->can_dlc = canfd_dlc_array[dlc_value - 8];
         }
+
+        if (FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, BRS)) {
+            frame->flags |= QEMU_CAN_FRMF_BRS;
+        }
     } else {
         /*
          * FD Format bit not set that means it is a CAN Frame.
@@ -1058,6 +1064,13 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
                     dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, 8 + i);
                 }
             }
+
+            if (frame->flags & QEMU_CAN_FRMF_BRS) {
+                dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, BRS, 1);
+            }
+            if (frame->flags & QEMU_CAN_FRMF_ESI) {
+                dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, ESI, 1);
+            }
         } else {
             is_canfd_frame = false;
 
-- 
2.34.1

[PATCH v2 5/7] hw/net/can/xlnx-versal-canfd: Fix byte ordering

[Doug Brown]

The endianness of the CAN data was backwards in each group of 4 bytes.
For example, the following data:

00 11 22 33 44 55 66 77

was showing up like this:

33 22 11 00 77 66 55 44

Fix both the TX and RX code to put the data in the correct order.

Signed-off-by: Doug Brown <doug@schmorgal.com>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
---
 hw/net/can/xlnx-versal-canfd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 47631917ab..5d7adf8740 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -951,7 +951,7 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
     }
 
     for (j = 0; j < frame->can_dlc; j++) {
-        val = 8 * i;
+        val = 8 * (3 - i);
 
         frame->data[j] = extract32(s->regs[reg_num + 2 + (j / 4)], val, 8);
         i++;
@@ -1093,19 +1093,19 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
             case 0:
                 rx_reg_num = i / 4;
 
-                data_reg_val = FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES3,
+                data_reg_val = FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES0,
                                           frame->data[i]);
                 break;
             case 1:
-                data_reg_val |= FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES2,
+                data_reg_val |= FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES1,
                                            frame->data[i]);
                 break;
             case 2:
-                data_reg_val |= FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES1,
+                data_reg_val |= FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES2,
                                            frame->data[i]);
                 break;
             case 3:
-                data_reg_val |= FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES0,
+                data_reg_val |= FIELD_DP32(0, RB_DW0_REGISTER, DATA_BYTES3,
                                            frame->data[i]);
                 /*
                  * Last Bytes data which means we have all 4 bytes ready to
-- 
2.34.1

[PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions

[Doug Brown]

Use QEMU's helper functions can_dlc2len() and can_len2dlc() for
translating between the raw DLC value and the SocketCAN length value.
This also has the side effect of correctly handling received CAN FD
frames with a DLC of 0-8, which was broken previously.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 hw/net/can/xlnx-versal-canfd.c | 67 ++--------------------------------
 1 file changed, 4 insertions(+), 63 deletions(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 5d7adf8740..589c21db69 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -678,8 +678,6 @@ REG32(RB_DW15_REGISTER_1, 0x4144)
     FIELD(RB_DW15_REGISTER_1, DATA_BYTES62, 8, 8)
     FIELD(RB_DW15_REGISTER_1, DATA_BYTES63, 0, 8)
 
-static uint8_t canfd_dlc_array[8] = {8, 12, 16, 20, 24, 32, 48, 64};
-
 static void canfd_update_irq(XlnxVersalCANFDState *s)
 {
     const bool irq = (s->regs[R_INTERRUPT_STATUS_REGISTER] &
@@ -897,59 +895,19 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
     }
 
     if (FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, FDF)) {
-        /*
-         * CANFD frame.
-         * Converting dlc(0 to 15) 4 Byte data to plain length(i.e. 0 to 64)
-         * 1 Byte data. This is done to make it work with SocketCAN.
-         * On actual CANFD frame, this value can't be more than 0xF.
-         * Conversion table for DLC to plain length:
-         *
-         *  DLC                        Plain Length
-         *  0 - 8                      0 - 8
-         *  9                          9 - 12
-         *  10                         13 - 16
-         *  11                         17 - 20
-         *  12                         21 - 24
-         *  13                         25 - 32
-         *  14                         33 - 48
-         *  15                         49 - 64
-         */
-
         frame->flags |= QEMU_CAN_FRMF_TYPE_FD;
 
-        if (dlc_value < 8) {
-            frame->can_dlc = dlc_value;
-        } else {
-            assert((dlc_value - 8) < ARRAY_SIZE(canfd_dlc_array));
-            frame->can_dlc = canfd_dlc_array[dlc_value - 8];
-        }
-
         if (FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, BRS)) {
             frame->flags |= QEMU_CAN_FRMF_BRS;
         }
     } else {
-        /*
-         * FD Format bit not set that means it is a CAN Frame.
-         * Conversion table for classic CAN:
-         *
-         *  DLC                        Plain Length
-         *  0 - 7                      0 - 7
-         *  8 - 15                     8
-         */
-
-        if (dlc_value > 8) {
-            frame->can_dlc = 8;
-            qemu_log_mask(LOG_GUEST_ERROR, "Maximum DLC value for Classic CAN"
-                          " frame is 8. Only 8 byte data will be sent.\n");
-        } else {
-            frame->can_dlc = dlc_value;
-        }
-
         if (is_rtr) {
             frame->can_id |= QEMU_CAN_RTR_FLAG;
         }
     }
 
+    frame->can_dlc = can_dlc2len(dlc_value);
+
     for (j = 0; j < frame->can_dlc; j++) {
         val = 8 * (3 - i);
 
@@ -1007,7 +965,6 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
                                 bool rx_fifo_id, uint8_t filter_index)
 {
     int i;
-    bool is_canfd_frame;
     uint8_t dlc = frame->can_dlc;
     uint8_t rx_reg_num = 0;
     uint32_t dlc_reg_val = 0;
@@ -1053,17 +1010,10 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
 
         s->regs[store_location] = frame_to_reg_id(frame);
 
-        dlc = frame->can_dlc;
+        dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, can_len2dlc(dlc));
 
         if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
-            is_canfd_frame = true;
-
-            /* Store dlc value in Xilinx specific format. */
-            for (i = 0; i < ARRAY_SIZE(canfd_dlc_array); i++) {
-                if (canfd_dlc_array[i] == frame->can_dlc) {
-                    dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, 8 + i);
-                }
-            }
+            dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, FDF, 1);
 
             if (frame->flags & QEMU_CAN_FRMF_BRS) {
                 dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, BRS, 1);
@@ -1071,17 +1021,8 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
             if (frame->flags & QEMU_CAN_FRMF_ESI) {
                 dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, ESI, 1);
             }
-        } else {
-            is_canfd_frame = false;
-
-            if (frame->can_dlc > 8) {
-                dlc = 8;
-            }
-
-            dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, dlc);
         }
 
-        dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, FDF, is_canfd_frame);
         dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, TIMESTAMP, rx_timestamp);
         dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, MATCHED_FILTER_INDEX,
                                   filter_index);
-- 
2.34.1

[PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues

[Doug Brown]

The read index should not be changed when storing a new message into the
RX or TX FIFO. Changing it at this point will cause the reader to get
out of sync. The wrapping of the read index is already handled by the
pre-write functions for the FIFO status registers anyway.

Additionally, the calculation for wrapping the store index was off by
one, which caused new messages to be written to the wrong location in
the FIFO. This caused incorrect messages to be delivered.

Signed-off-by: Doug Brown <doug@schmorgal.com>
---
 hw/net/can/xlnx-versal-canfd.c | 36 +++-------------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index 589c21db69..c291a0272b 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -1144,18 +1144,8 @@ static void update_rx_sequential(XlnxVersalCANFDState *s,
             read_index = ARRAY_FIELD_EX32(s->regs, RX_FIFO_STATUS_REGISTER, RI);
             store_index = read_index + fill_level;
 
-            if (read_index == s->cfg.rx0_fifo - 1) {
-                /*
-                 * When ri is s->cfg.rx0_fifo - 1 i.e. max, it goes cyclic that
-                 * means we reset the ri to 0x0.
-                 */
-                read_index = 0;
-                ARRAY_FIELD_DP32(s->regs, RX_FIFO_STATUS_REGISTER, RI,
-                                 read_index);
-            }
-
             if (store_index > s->cfg.rx0_fifo - 1) {
-                store_index -= s->cfg.rx0_fifo - 1;
+                store_index -= s->cfg.rx0_fifo;
             }
 
             store_location = R_RB_ID_REGISTER +
@@ -1172,18 +1162,8 @@ static void update_rx_sequential(XlnxVersalCANFDState *s,
                                           RI_1);
             store_index = read_index + fill_level;
 
-            if (read_index == s->cfg.rx1_fifo - 1) {
-                /*
-                 * When ri is s->cfg.rx1_fifo - 1 i.e. max, it goes cyclic that
-                 * means we reset the ri to 0x0.
-                 */
-                read_index = 0;
-                ARRAY_FIELD_DP32(s->regs, RX_FIFO_STATUS_REGISTER, RI_1,
-                                 read_index);
-            }
-
             if (store_index > s->cfg.rx1_fifo - 1) {
-                store_index -= s->cfg.rx1_fifo - 1;
+                store_index -= s->cfg.rx1_fifo;
             }
 
             store_location = R_RB_ID_REGISTER_1 +
@@ -1265,18 +1245,8 @@ static void tx_fifo_stamp(XlnxVersalCANFDState *s, uint32_t tb0_regid)
                           " Discarding the message\n");
             ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, TXEOFLW, 1);
         } else {
-            if (read_index == s->cfg.tx_fifo - 1) {
-                /*
-                 * When ri is s->cfg.tx_fifo - 1 i.e. max, it goes cyclic that
-                 * means we reset the ri to 0x0.
-                 */
-                read_index = 0;
-                ARRAY_FIELD_DP32(s->regs, TX_EVENT_FIFO_STATUS_REGISTER, TXE_RI,
-                                 read_index);
-            }
-
             if (store_index > s->cfg.tx_fifo - 1) {
-                store_index -= s->cfg.tx_fifo - 1;
+                store_index -= s->cfg.tx_fifo;
             }
 
             assert(store_index < s->cfg.tx_fifo);
-- 
2.34.1

Re: [PATCH v2 1/7] hw/net/can/xlnx-versal-canfd: Fix interrupt level

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:21 Doug Brown wrote:
> The interrupt level should be 0 or 1. The existing code was using the
> interrupt flags to determine the level. In the only machine currently
> supported (xlnx-versal-virt), the GICv3 was masking off all bits except
> bit 0 when applying it, resulting in the IRQ never being delivered.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>


-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 2/7] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:22 Doug Brown wrote:
> When checking the QEMU_CAN_FRMF_TYPE_FD flag, we need to ignore other
> potentially set flags. Before this change, received CAN FD frames from
> SocketCAN weren't being recognized as CAN FD.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 3/7] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:23 Doug Brown wrote:
> Previously the emulated CAN ID register was being set to the exact same
> value stored in qemu_can_frame.can_id. This doesn't work correctly
> because the Xilinx IP core uses a different bit arrangement than
> qemu_can_frame for all of its ID registers. Correct this problem for
> both RX and TX, including RX filtering.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 4/7] hw/net/can/xlnx-versal-canfd: Handle flags correctly

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:24 Doug Brown wrote:
> Add support for QEMU_CAN_FRMF_ESI and QEMU_CAN_FRMF_BRS flags, and
> ensure frame->flags is always initialized to 0.
>
> Note that the Xilinx IP core doesn't allow manually setting the ESI bit
> during transmits, so it's only implemented for the receive case.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 5/7] hw/net/can/xlnx-versal-canfd: Fix byte ordering

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:25 Doug Brown wrote:
> The endianness of the CAN data was backwards in each group of 4 bytes.
> For example, the following data:
>
> 00 11 22 33 44 55 66 77
>
> was showing up like this:
>
> 33 22 11 00 77 66 55 44
>
> Fix both the TX and RX code to put the data in the correct order.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:26 Doug Brown wrote:
> Use QEMU's helper functions can_dlc2len() and can_len2dlc() for
> translating between the raw DLC value and the SocketCAN length value.
> This also has the side effect of correctly handling received CAN FD
> frames with a DLC of 0-8, which was broken previously.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues

[Pavel Pisa]

On Tuesday 27 of August 2024 05:49:27 Doug Brown wrote:
> The read index should not be changed when storing a new message into the
> RX or TX FIFO. Changing it at this point will cause the reader to get
> out of sync. The wrapping of the read index is already handled by the
> pre-write functions for the FIFO status registers anyway.
>
> Additionally, the calculation for wrapping the store index was off by
> one, which caused new messages to be written to the wrong location in
> the FIFO. This caused incorrect messages to be delivered.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Generally, I agree that index should wrap up for cyclic FIFO
implementation and change looks logical to me but I do not
see and studied all consequences related to emulated HW.

If that is confirmed or corrected by somebody from AMD/XilinX,
it would be better. I can find more time to do deeper analysis
if no other looks into the whole code.

Best wishes,

                Pavel
--
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues

[Doug Brown]

Hi Pavel,

On 8/29/2024 6:24 AM, Pavel Pisa wrote:
> Generally, I agree that index should wrap up for cyclic FIFO
> implementation and change looks logical to me but I do not
> see and studied all consequences related to emulated HW.
> 
> If that is confirmed or corrected by somebody from AMD/XilinX,
> it would be better. I can find more time to do deeper analysis
> if no other looks into the whole code.

Thank you for your reviews! I agree. I don't know how the hardware is
actually implemented internally so it would be great if someone from AMD
could confirm that this change 100% matches the hardware. Unfortunately
Versal dev boards are very expensive so I can't test hardware myself.

What I can say is that before I implemented the index calculation fixes
in this patch, the Linux driver in the Xilinx kernel was seeing
incorrect messages. Now it works fine even with heavy bus load.

Doug

Re: [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions

[Francisco Iglesias]

On Mon, Aug 26, 2024 at 08:49:26PM -0700, Doug Brown wrote:
> Use QEMU's helper functions can_dlc2len() and can_len2dlc() for
> translating between the raw DLC value and the SocketCAN length value.
> This also has the side effect of correctly handling received CAN FD
> frames with a DLC of 0-8, which was broken previously.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>


> ---
>  hw/net/can/xlnx-versal-canfd.c | 67 ++--------------------------------
>  1 file changed, 4 insertions(+), 63 deletions(-)
> 
> diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
> index 5d7adf8740..589c21db69 100644
> --- a/hw/net/can/xlnx-versal-canfd.c
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -678,8 +678,6 @@ REG32(RB_DW15_REGISTER_1, 0x4144)
>      FIELD(RB_DW15_REGISTER_1, DATA_BYTES62, 8, 8)
>      FIELD(RB_DW15_REGISTER_1, DATA_BYTES63, 0, 8)
>  
> -static uint8_t canfd_dlc_array[8] = {8, 12, 16, 20, 24, 32, 48, 64};
> -
>  static void canfd_update_irq(XlnxVersalCANFDState *s)
>  {
>      const bool irq = (s->regs[R_INTERRUPT_STATUS_REGISTER] &
> @@ -897,59 +895,19 @@ static void regs2frame(XlnxVersalCANFDState *s, qemu_can_frame *frame,
>      }
>  
>      if (FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, FDF)) {
> -        /*
> -         * CANFD frame.
> -         * Converting dlc(0 to 15) 4 Byte data to plain length(i.e. 0 to 64)
> -         * 1 Byte data. This is done to make it work with SocketCAN.
> -         * On actual CANFD frame, this value can't be more than 0xF.
> -         * Conversion table for DLC to plain length:
> -         *
> -         *  DLC                        Plain Length
> -         *  0 - 8                      0 - 8
> -         *  9                          9 - 12
> -         *  10                         13 - 16
> -         *  11                         17 - 20
> -         *  12                         21 - 24
> -         *  13                         25 - 32
> -         *  14                         33 - 48
> -         *  15                         49 - 64
> -         */
> -
>          frame->flags |= QEMU_CAN_FRMF_TYPE_FD;
>  
> -        if (dlc_value < 8) {
> -            frame->can_dlc = dlc_value;
> -        } else {
> -            assert((dlc_value - 8) < ARRAY_SIZE(canfd_dlc_array));
> -            frame->can_dlc = canfd_dlc_array[dlc_value - 8];
> -        }
> -
>          if (FIELD_EX32(dlc_reg_val, TB0_DLC_REGISTER, BRS)) {
>              frame->flags |= QEMU_CAN_FRMF_BRS;
>          }
>      } else {
> -        /*
> -         * FD Format bit not set that means it is a CAN Frame.
> -         * Conversion table for classic CAN:
> -         *
> -         *  DLC                        Plain Length
> -         *  0 - 7                      0 - 7
> -         *  8 - 15                     8
> -         */
> -
> -        if (dlc_value > 8) {
> -            frame->can_dlc = 8;
> -            qemu_log_mask(LOG_GUEST_ERROR, "Maximum DLC value for Classic CAN"
> -                          " frame is 8. Only 8 byte data will be sent.\n");
> -        } else {
> -            frame->can_dlc = dlc_value;
> -        }
> -
>          if (is_rtr) {
>              frame->can_id |= QEMU_CAN_RTR_FLAG;
>          }
>      }
>  
> +    frame->can_dlc = can_dlc2len(dlc_value);
> +
>      for (j = 0; j < frame->can_dlc; j++) {
>          val = 8 * (3 - i);
>  
> @@ -1007,7 +965,6 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
>                                  bool rx_fifo_id, uint8_t filter_index)
>  {
>      int i;
> -    bool is_canfd_frame;
>      uint8_t dlc = frame->can_dlc;
>      uint8_t rx_reg_num = 0;
>      uint32_t dlc_reg_val = 0;
> @@ -1053,17 +1010,10 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
>  
>          s->regs[store_location] = frame_to_reg_id(frame);
>  
> -        dlc = frame->can_dlc;
> +        dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, can_len2dlc(dlc));
>  
>          if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
> -            is_canfd_frame = true;
> -
> -            /* Store dlc value in Xilinx specific format. */
> -            for (i = 0; i < ARRAY_SIZE(canfd_dlc_array); i++) {
> -                if (canfd_dlc_array[i] == frame->can_dlc) {
> -                    dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, 8 + i);
> -                }
> -            }
> +            dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, FDF, 1);
>  
>              if (frame->flags & QEMU_CAN_FRMF_BRS) {
>                  dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, BRS, 1);
> @@ -1071,17 +1021,8 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
>              if (frame->flags & QEMU_CAN_FRMF_ESI) {
>                  dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, ESI, 1);
>              }
> -        } else {
> -            is_canfd_frame = false;
> -
> -            if (frame->can_dlc > 8) {
> -                dlc = 8;
> -            }
> -
> -            dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, dlc);
>          }
>  
> -        dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, FDF, is_canfd_frame);
>          dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, TIMESTAMP, rx_timestamp);
>          dlc_reg_val |= FIELD_DP32(0, RB_DLC_REGISTER, MATCHED_FILTER_INDEX,
>                                    filter_index);
> -- 
> 2.34.1
> 

Re: [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes

[Peter Maydell]

On Tue, 27 Aug 2024 at 04:51, Doug Brown <doug@schmorgal.com> wrote:
>
> This series fixes several problems I ran into while trying to simulate
> the AMD/Xilinx Versal CANFD controller in the xlnx-versal-virt machine
> using Xilinx's v6.6_LTS_2024.1 kernel. With all of these patches
> applied, everything works correctly alongside actual CAN devices.

Couple of Qs for the Xilinx/AMD folks:
 (1) do you intend to review patch 7 of this series? That's
     the one unreviewed one and it could use a look from
     somebody familiar with how the versal canfd h/w works.
     If you don't I propose to apply this series as-is next week.
 (2) it sounds like Vikram Garhwal's AMD email is bouncing --
     do you want to nominate somebody else to take his place
     as co-maintainer/reviewer of CAN bus stuff?
     If not, we can just drop his lines from MAINTAINERS.

> - IRQs were accidentally not being delivered due to having a level other
>   than 1. The IRQ count in /proc/interrupts in Linux was stuck at 0.
> - Incoming CAN FD frames were being treated as non-FD.
> - The CAN IDs were garbled in both RX and TX directions.
> - The ESI and BRS flags were not being handled.
> - The byte ordering was wrong in the data in both directions.
> - Incoming CAN FD frames with DLC = 1-7 weren't handled correctly.
> - The FIFO read_index and store_index wrapping logic was incorrect.
>
> I don't have any actual Versal hardware to compare behavior against, but
> with these changes, it plays nicely with SocketCAN on the host system.
>
> Changes in v2:
> - Added handling of ESI and BRS flags, ensured frame->flags is initialized
> - Switched to use common can_dlc2len() and can_len2dlc() functions
> - Added fix for FIFO wrapping problems I observed during stress testing
>
> Doug Brown (7):
>   hw/net/can/xlnx-versal-canfd: Fix interrupt level
>   hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check
>   hw/net/can/xlnx-versal-canfd: Translate CAN ID registers
>   hw/net/can/xlnx-versal-canfd: Handle flags correctly
>   hw/net/can/xlnx-versal-canfd: Fix byte ordering
>   hw/net/can/xlnx-versal-canfd: Simplify DLC conversions
>   hw/net/can/xlnx-versal-canfd: Fix FIFO issues
>
>  hw/net/can/xlnx-versal-canfd.c | 173 ++++++++++++++-------------------
>  1 file changed, 72 insertions(+), 101 deletions(-)

thanks
-- PMM

Re: [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes

[Francisco Iglesias]

Hi Peter,

On Fri, Sep 06, 2024 at 03:36:10PM +0100, Peter Maydell wrote:
> On Tue, 27 Aug 2024 at 04:51, Doug Brown <doug@schmorgal.com> wrote:
> >
> > This series fixes several problems I ran into while trying to simulate
> > the AMD/Xilinx Versal CANFD controller in the xlnx-versal-virt machine
> > using Xilinx's v6.6_LTS_2024.1 kernel. With all of these patches
> > applied, everything works correctly alongside actual CAN devices.
> 
> Couple of Qs for the Xilinx/AMD folks:
>  (1) do you intend to review patch 7 of this series? That's
>      the one unreviewed one and it could use a look from
>      somebody familiar with how the versal canfd h/w works.
>      If you don't I propose to apply this series as-is next week.

I've started reviewing it (but got interrupted), I'll comeback later today.

>  (2) it sounds like Vikram Garhwal's AMD email is bouncing --
>      do you want to nominate somebody else to take his place
>      as co-maintainer/reviewer of CAN bus stuff?
>      If not, we can just drop his lines from MAINTAINERS.

I can take Vikram's place, I'll post a patch with the update.

Best regards,
Francisco

> 
> > - IRQs were accidentally not being delivered due to having a level other
> >   than 1. The IRQ count in /proc/interrupts in Linux was stuck at 0.
> > - Incoming CAN FD frames were being treated as non-FD.
> > - The CAN IDs were garbled in both RX and TX directions.
> > - The ESI and BRS flags were not being handled.
> > - The byte ordering was wrong in the data in both directions.
> > - Incoming CAN FD frames with DLC = 1-7 weren't handled correctly.
> > - The FIFO read_index and store_index wrapping logic was incorrect.
> >
> > I don't have any actual Versal hardware to compare behavior against, but
> > with these changes, it plays nicely with SocketCAN on the host system.
> >
> > Changes in v2:
> > - Added handling of ESI and BRS flags, ensured frame->flags is initialized
> > - Switched to use common can_dlc2len() and can_len2dlc() functions
> > - Added fix for FIFO wrapping problems I observed during stress testing
> >
> > Doug Brown (7):
> >   hw/net/can/xlnx-versal-canfd: Fix interrupt level
> >   hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check
> >   hw/net/can/xlnx-versal-canfd: Translate CAN ID registers
> >   hw/net/can/xlnx-versal-canfd: Handle flags correctly
> >   hw/net/can/xlnx-versal-canfd: Fix byte ordering
> >   hw/net/can/xlnx-versal-canfd: Simplify DLC conversions
> >   hw/net/can/xlnx-versal-canfd: Fix FIFO issues
> >
> >  hw/net/can/xlnx-versal-canfd.c | 173 ++++++++++++++-------------------
> >  1 file changed, 72 insertions(+), 101 deletions(-)
> 
> thanks
> -- PMM

Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues

[Francisco Iglesias]

Hi Doug,

On Mon, Aug 26, 2024 at 08:49:27PM -0700, Doug Brown wrote:
> The read index should not be changed when storing a new message into the
> RX or TX FIFO. Changing it at this point will cause the reader to get
> out of sync. The wrapping of the read index is already handled by the
> pre-write functions for the FIFO status registers anyway.
> 
> Additionally, the calculation for wrapping the store index was off by
> one, which caused new messages to be written to the wrong location in
> the FIFO. This caused incorrect messages to be delivered.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

Thanks a lot for the fixes and sorry for the delayed review!

Best regards,
Francisco


> ---
>  hw/net/can/xlnx-versal-canfd.c | 36 +++-------------------------------
>  1 file changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
> index 589c21db69..c291a0272b 100644
> --- a/hw/net/can/xlnx-versal-canfd.c
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -1144,18 +1144,8 @@ static void update_rx_sequential(XlnxVersalCANFDState *s,
>              read_index = ARRAY_FIELD_EX32(s->regs, RX_FIFO_STATUS_REGISTER, RI);
>              store_index = read_index + fill_level;
>  
> -            if (read_index == s->cfg.rx0_fifo - 1) {
> -                /*
> -                 * When ri is s->cfg.rx0_fifo - 1 i.e. max, it goes cyclic that
> -                 * means we reset the ri to 0x0.
> -                 */
> -                read_index = 0;
> -                ARRAY_FIELD_DP32(s->regs, RX_FIFO_STATUS_REGISTER, RI,
> -                                 read_index);
> -            }
> -
>              if (store_index > s->cfg.rx0_fifo - 1) {
> -                store_index -= s->cfg.rx0_fifo - 1;
> +                store_index -= s->cfg.rx0_fifo;
>              }
>  
>              store_location = R_RB_ID_REGISTER +
> @@ -1172,18 +1162,8 @@ static void update_rx_sequential(XlnxVersalCANFDState *s,
>                                            RI_1);
>              store_index = read_index + fill_level;
>  
> -            if (read_index == s->cfg.rx1_fifo - 1) {
> -                /*
> -                 * When ri is s->cfg.rx1_fifo - 1 i.e. max, it goes cyclic that
> -                 * means we reset the ri to 0x0.
> -                 */
> -                read_index = 0;
> -                ARRAY_FIELD_DP32(s->regs, RX_FIFO_STATUS_REGISTER, RI_1,
> -                                 read_index);
> -            }
> -
>              if (store_index > s->cfg.rx1_fifo - 1) {
> -                store_index -= s->cfg.rx1_fifo - 1;
> +                store_index -= s->cfg.rx1_fifo;
>              }
>  
>              store_location = R_RB_ID_REGISTER_1 +
> @@ -1265,18 +1245,8 @@ static void tx_fifo_stamp(XlnxVersalCANFDState *s, uint32_t tb0_regid)
>                            " Discarding the message\n");
>              ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, TXEOFLW, 1);
>          } else {
> -            if (read_index == s->cfg.tx_fifo - 1) {
> -                /*
> -                 * When ri is s->cfg.tx_fifo - 1 i.e. max, it goes cyclic that
> -                 * means we reset the ri to 0x0.
> -                 */
> -                read_index = 0;
> -                ARRAY_FIELD_DP32(s->regs, TX_EVENT_FIFO_STATUS_REGISTER, TXE_RI,
> -                                 read_index);
> -            }
> -
>              if (store_index > s->cfg.tx_fifo - 1) {
> -                store_index -= s->cfg.tx_fifo - 1;
> +                store_index -= s->cfg.tx_fifo;
>              }
>  
>              assert(store_index < s->cfg.tx_fifo);
> -- 
> 2.34.1
> 

Re: [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes

[Peter Maydell]

On Tue, 27 Aug 2024 at 04:51, Doug Brown <doug@schmorgal.com> wrote:
>
> This series fixes several problems I ran into while trying to simulate
> the AMD/Xilinx Versal CANFD controller in the xlnx-versal-virt machine
> using Xilinx's v6.6_LTS_2024.1 kernel. With all of these patches
> applied, everything works correctly alongside actual CAN devices.
>
> - IRQs were accidentally not being delivered due to having a level other
>   than 1. The IRQ count in /proc/interrupts in Linux was stuck at 0.
> - Incoming CAN FD frames were being treated as non-FD.
> - The CAN IDs were garbled in both RX and TX directions.
> - The ESI and BRS flags were not being handled.
> - The byte ordering was wrong in the data in both directions.
> - Incoming CAN FD frames with DLC = 1-7 weren't handled correctly.
> - The FIFO read_index and store_index wrapping logic was incorrect.
>
> I don't have any actual Versal hardware to compare behavior against, but
> with these changes, it plays nicely with SocketCAN on the host system.
>
> Changes in v2:
> - Added handling of ESI and BRS flags, ensured frame->flags is initialized
> - Switched to use common can_dlc2len() and can_len2dlc() functions
> - Added fix for FIFO wrapping problems I observed during stress testing

I've applied this series to target-arm.next; thanks!

-- PMM