* [PATCH v2 1/7] hw/net/can/xlnx-versal-canfd: Fix interrupt level
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:08 ` Pavel Pisa
2024-08-27 3:49 ` [PATCH v2 2/7] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check Doug Brown
` (7 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 2/7] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
2024-08-27 3:49 ` [PATCH v2 1/7] hw/net/can/xlnx-versal-canfd: Fix interrupt level Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:09 ` Pavel Pisa
2024-08-27 3:49 ` [PATCH v2 3/7] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers Doug Brown
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 3/7] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
2024-08-27 3:49 ` [PATCH v2 1/7] hw/net/can/xlnx-versal-canfd: Fix interrupt level Doug Brown
2024-08-27 3:49 ` [PATCH v2 2/7] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:11 ` Pavel Pisa
2024-08-27 3:49 ` [PATCH v2 4/7] hw/net/can/xlnx-versal-canfd: Handle flags correctly Doug Brown
` (5 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 4/7] hw/net/can/xlnx-versal-canfd: Handle flags correctly
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
` (2 preceding siblings ...)
2024-08-27 3:49 ` [PATCH v2 3/7] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:13 ` Pavel Pisa
2024-08-27 3:49 ` [PATCH v2 5/7] hw/net/can/xlnx-versal-canfd: Fix byte ordering Doug Brown
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 5/7] hw/net/can/xlnx-versal-canfd: Fix byte ordering
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
` (3 preceding siblings ...)
2024-08-27 3:49 ` [PATCH v2 4/7] hw/net/can/xlnx-versal-canfd: Handle flags correctly Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:15 ` Pavel Pisa
2024-08-27 3:49 ` [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions Doug Brown
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
` (4 preceding siblings ...)
2024-08-27 3:49 ` [PATCH v2 5/7] hw/net/can/xlnx-versal-canfd: Fix byte ordering Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:17 ` Pavel Pisa
2024-09-04 23:33 ` Francisco Iglesias
2024-08-27 3:49 ` [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues Doug Brown
` (2 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions
2024-08-27 3:49 ` [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions Doug Brown
@ 2024-08-29 13:17 ` Pavel Pisa
2024-09-04 23:33 ` Francisco Iglesias
1 sibling, 0 replies; 21+ messages in thread
From: Pavel Pisa @ 2024-08-29 13:17 UTC (permalink / raw)
To: Doug Brown
Cc: Francisco Iglesias, Jason Wang, Paolo Bonzini, qemu-devel,
Peter Maydell
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions
2024-08-27 3:49 ` [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions Doug Brown
2024-08-29 13:17 ` Pavel Pisa
@ 2024-09-04 23:33 ` Francisco Iglesias
1 sibling, 0 replies; 21+ messages in thread
From: Francisco Iglesias @ 2024-09-04 23:33 UTC (permalink / raw)
To: Doug Brown; +Cc: Pavel Pisa, Jason Wang, Paolo Bonzini, qemu-devel
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
` (5 preceding siblings ...)
2024-08-27 3:49 ` [PATCH v2 6/7] hw/net/can/xlnx-versal-canfd: Simplify DLC conversions Doug Brown
@ 2024-08-27 3:49 ` Doug Brown
2024-08-29 13:24 ` Pavel Pisa
2024-09-06 16:35 ` Francisco Iglesias
2024-09-06 14:36 ` [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Peter Maydell
2024-09-09 13:55 ` Peter Maydell
8 siblings, 2 replies; 21+ messages in thread
From: Doug Brown @ 2024-08-27 3:49 UTC (permalink / raw)
To: Francisco Iglesias, Pavel Pisa
Cc: Jason Wang, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues
2024-08-27 3:49 ` [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues Doug Brown
@ 2024-08-29 13:24 ` Pavel Pisa
2024-08-30 0:11 ` Doug Brown
2024-09-06 16:35 ` Francisco Iglesias
1 sibling, 1 reply; 21+ messages in thread
From: Pavel Pisa @ 2024-08-29 13:24 UTC (permalink / raw)
To: Doug Brown
Cc: Francisco Iglesias, Jason Wang, Paolo Bonzini, qemu-devel,
Peter Maydell
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues
2024-08-29 13:24 ` Pavel Pisa
@ 2024-08-30 0:11 ` Doug Brown
0 siblings, 0 replies; 21+ messages in thread
From: Doug Brown @ 2024-08-30 0:11 UTC (permalink / raw)
To: Pavel Pisa
Cc: Francisco Iglesias, Jason Wang, Paolo Bonzini, qemu-devel,
Peter Maydell
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues
2024-08-27 3:49 ` [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues Doug Brown
2024-08-29 13:24 ` Pavel Pisa
@ 2024-09-06 16:35 ` Francisco Iglesias
1 sibling, 0 replies; 21+ messages in thread
From: Francisco Iglesias @ 2024-09-06 16:35 UTC (permalink / raw)
To: Doug Brown; +Cc: Pavel Pisa, Jason Wang, Paolo Bonzini, qemu-devel
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
` (6 preceding siblings ...)
2024-08-27 3:49 ` [PATCH v2 7/7] hw/net/can/xlnx-versal-canfd: Fix FIFO issues Doug Brown
@ 2024-09-06 14:36 ` Peter Maydell
2024-09-06 15:48 ` Francisco Iglesias
2024-09-09 13:55 ` Peter Maydell
8 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2024-09-06 14:36 UTC (permalink / raw)
To: Doug Brown
Cc: Francisco Iglesias, Pavel Pisa, qemu-devel, Edgar E. Iglesias,
Alistair Francis
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes
2024-09-06 14:36 ` [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Peter Maydell
@ 2024-09-06 15:48 ` Francisco Iglesias
0 siblings, 0 replies; 21+ messages in thread
From: Francisco Iglesias @ 2024-09-06 15:48 UTC (permalink / raw)
To: Peter Maydell
Cc: Doug Brown, Pavel Pisa, qemu-devel, Edgar E. Iglesias,
Alistair Francis
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes
2024-08-27 3:49 [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Doug Brown
` (7 preceding siblings ...)
2024-09-06 14:36 ` [PATCH v2 0/7] hw/net/can/xlnx-versal-canfd: Miscellaneous fixes Peter Maydell
@ 2024-09-09 13:55 ` Peter Maydell
8 siblings, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2024-09-09 13:55 UTC (permalink / raw)
To: Doug Brown
Cc: Francisco Iglesias, Pavel Pisa, Jason Wang, Paolo Bonzini,
qemu-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread