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

[PATCH 0/5] 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 byte ordering was wrong in the data in both directions.
- Incoming CAN FD frames with DLC = 1-7 weren't handled correctly.

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.

Doug Brown (5):
  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: Fix byte ordering
  hw/net/can/xlnx-versal-canfd: Handle RX of short FD frames

 hw/net/can/xlnx-versal-canfd.c | 82 +++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/5] 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>
---
 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 2/5] 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>
---
 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 3/5] 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>
---
 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 4/5] 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>
---
 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 1704b558d0..fda1e7016a 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -945,7 +945,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++;
@@ -1080,19 +1080,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 5/5] hw/net/can/xlnx-versal-canfd: Handle RX of short FD frames

[Doug Brown]

There was no case for handling received CAN FD frames with a DLC of 0-8.
This was already handled properly with TX. Add similar code for RX.

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

diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
index fda1e7016a..ce68dab46a 100644
--- a/hw/net/can/xlnx-versal-canfd.c
+++ b/hw/net/can/xlnx-versal-canfd.c
@@ -1052,10 +1052,15 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
         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);
+            if (dlc <= 8) {
+                dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, dlc);
+            } else {
+                /* 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);
+                    }
                 }
             }
         } else {
-- 
2.34.1

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

[Pavel Pisa]

Hello Doug Brown,

On Friday 16 of August 2024 18:35:02 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>
> ---
>  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. */

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

That is a great catch, I have overlooked this in previous
review of the Xilinx code.

When I look into hw/net/can/xlnx-versal-canfd.c functions
regs2frame and store_rx_sequential then I see missing
handling of flags QEMU_CAN_FRMF_ESI and QEMU_CAN_FRMF_BRS.

In the function regs2frame is missing even initialization
of frame->flags = 0 at the start, which I expect should be there.
The
  frame->flags = QEMU_CAN_FRMF_TYPE_FD;
should be then
  frame->flags |= QEMU_CAN_FRMF_TYPE_FD;

You can see how it was intended to parse and fill flags in our
CTU CAN FD interface code which matches our design of common
QEMU CAN infrastructure and its extension for CAN FD.

See the functions
  ctucan_buff2frame()
  ctucan_frame2buff()
in
  hw/net/can/ctucan_core.c

QEMU_CAN_EFF_FLAG and QEMU_CAN_RTR_FLAG seems to be corrected
in followup patch

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

As for the DLC conversion, there are functions

  frame->can_dlc = can_dlc2len(xxxx)
  XXX = can_len2dlc(frame->can_dlc);

provided by net/can/can_core.c

I am not sure how much competent I am for the rest of the patches,
because I do not know XilinX IP core so well. Review by Vikram Garhwal
or somebody else from AMD/XilinX is more valueable there.
But I can add my ACK there based on rough overview.

Best wishes,

                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 1/5] hw/net/can/xlnx-versal-canfd: Fix interrupt level

[Francisco Iglesias]

On Fri, Aug 16, 2024 at 09:35:01AM -0700, 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>

> ---
>  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
> 

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

[Francisco Iglesias]

On Fri, Aug 16, 2024 at 09:35:02AM -0700, 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: 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
> 

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

[Doug Brown]

Hi Pavel,

(Dropping Vikram from the email chain; I received "recipient not found"
errors from AMD's mail servers in response to all of my patches)

On 8/20/2024 11:57 PM, Pavel Pisa wrote:
> Hello Doug Brown,
> 
> On Friday 16 of August 2024 18:35:02 Doug Brown wrote:
>> -        if (frame->flags == QEMU_CAN_FRMF_TYPE_FD) {
>> +        if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
> 
> Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> 
> That is a great catch, I have overlooked this in previous
> review of the Xilinx code.

Thank you for reviewing!

> When I look into hw/net/can/xlnx-versal-canfd.c functions
> regs2frame and store_rx_sequential then I see missing
> handling of flags QEMU_CAN_FRMF_ESI and QEMU_CAN_FRMF_BRS.

Nice find. It looks like it would be pretty straightforward to add
support for those flags. As far as I can tell they map directly to
register bits.

> In the function regs2frame is missing even initialization
> of frame->flags = 0 at the start, which I expect should be there.
> The
>   frame->flags = QEMU_CAN_FRMF_TYPE_FD;
> should be then
>   frame->flags |= QEMU_CAN_FRMF_TYPE_FD;

You're absolutely right. It looks like frame->flags isn't being
initialized at all when it's a non-FD frame. I can also take care of
this. I'll wait and see how the review goes for the other patches, and I
can add another patch to fix these flag issues in the next version of
the series.

> As for the DLC conversion, there are functions
> 
>   frame->can_dlc = can_dlc2len(xxxx)
>   XXX = can_len2dlc(frame->can_dlc);
> 
> provided by net/can/can_core.c

Nice! It seems like using these functions could simplify this code quite
a bit, by eliminating the need for canfd_dlc_array. I can add this as
another patch for the next version.

> I am not sure how much competent I am for the rest of the patches,
> because I do not know XilinX IP core so well. Review by Vikram Garhwal
> or somebody else from AMD/XilinX is more valueable there.
> But I can add my ACK there based on rough overview.
Thanks! I also see that Francisco reviewed a couple of the patches --
thanks Francisco! I will wait and see how review goes on the others.

For what it's worth, I was stress testing this in QEMU today and found
another issue with the FIFO read_index and store_index calculations and
the logic that wraps them around to 0. I will submit the fix for this
problem as another patch in the next version of the series (or a new
series if that's more convenient).

Doug

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

[Pavel Pisa]

Hello Doug and Francisco,

thanks for cooperation

On Thursday 22 of August 2024 02:01:01 Doug Brown wrote:
> (Dropping Vikram from the email chain; I received "recipient not found"
> errors from AMD's mail servers in response to all of my patches)

If the address Vikram Garhwal <vikram.garhwal@amd.com> is not valid
then if he has still interest in CAN in QEMU it would be great
if he updates the address in the QEMU MAINTAINERS file. If he does
not plan to participate then the MAINTAINERS file should be updated
as well.

Vikram Garhwal is listed even as whole CAN subsystem comaintainer

  https://gitlab.com/qemu-project/qemu/-/blob/3472f54522a928f0020d6928d54c007f862c5478/MAINTAINERS#L2690

I plan take an eye on the system long term and I have
ideas and plans how to enhance it in more directions when
I find some spare time or project, studnets, thesis,
company interested in adding another controller etc.

But I would be more comfortable if there is somebody else
who is willing to be at least my backup when I am on some
trip without Internet (hiking etc.). I am quite loaded by
teaching etc. and all these my CAN and in the fact all my
open-source and other development projects are mostly out
of any interrest of the university department where I serve.
They would care a little if/when I bring paid contract from
some company, as we have from Volkswagen and its subsidiaries.
But it is long time ago at university and even longer
at my company.

So all depends on my enthusiasm and free time which
should have at least some maintainership backup by somebody
who intend to use the project in frame of company or some
automotive consortium. I know that there are big money flowing
on base of these activities.

Best wishes,


                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 3/5] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers

[Francisco Iglesias]

On Fri, Aug 16, 2024 at 09:35:03AM -0700, 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>


> ---
>  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
> 

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

[Francisco Iglesias]

On Fri, Aug 16, 2024 at 09:35:04AM -0700, 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>


> ---
>  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 1704b558d0..fda1e7016a 100644
> --- a/hw/net/can/xlnx-versal-canfd.c
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -945,7 +945,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++;
> @@ -1080,19 +1080,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
> 

Re: [PATCH 5/5] hw/net/can/xlnx-versal-canfd: Handle RX of short FD frames

[Francisco Iglesias]

On Fri, Aug 16, 2024 at 09:35:05AM -0700, Doug Brown wrote:
> There was no case for handling received CAN FD frames with a DLC of 0-8.
> This was already handled properly with TX. Add similar code for RX.
> 
> Signed-off-by: Doug Brown <doug@schmorgal.com>

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


> ---
>  hw/net/can/xlnx-versal-canfd.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
> index fda1e7016a..ce68dab46a 100644
> --- a/hw/net/can/xlnx-versal-canfd.c
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -1052,10 +1052,15 @@ static void store_rx_sequential(XlnxVersalCANFDState *s,
>          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);
> +            if (dlc <= 8) {
> +                dlc_reg_val = FIELD_DP32(0, RB_DLC_REGISTER, DLC, dlc);
> +            } else {
> +                /* 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);
> +                    }
>                  }
>              }
>          } else {
> -- 
> 2.34.1
> 

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

[Doug Brown]

Hello Pavel and Francisco,

On 8/21/2024 6:11 PM, Pavel Pisa wrote:
> Vikram Garhwal is listed even as whole CAN subsystem comaintainer
> 
>   https://gitlab.com/qemu-project/qemu/-/blob/3472f54522a928f0020d6928d54c007f862c5478/MAINTAINERS#L2690

Unfortunately I am totally new to submitting patches for QEMU and have
no connection with AMD so I can't speak about Vikram's situation. I
agree though, at the very least something needs to change in the
MAINTAINERS file. Maybe Francisco knows more?

> But I would be more comfortable if there is somebody else
> who is willing to be at least my backup when I am on some
> trip without Internet (hiking etc.). I am quite loaded by
> teaching etc. and all these my CAN and in the fact all my
> open-source and other development projects are mostly out
> of any interrest of the university department where I serve.
> They would care a little if/when I bring paid contract from
> some company, as we have from Volkswagen and its subsidiaries.
> But it is long time ago at university and even longer
> at my company.

I'm very, very new to QEMU development so I think I would be a poor fit
as a maintainer, at least right now. I'm also not affiliated with any
big money companies. I can completely understand that the subsystem
needs an additional maintainer though. That's a lot on one person's
shoulders!

Thank you for reviewing all of my patches, Francisco.

Now, all of these patches are reviewed but there are a few other issues
we talked about here (dlc2len/len2dlc and issues with the flags), and I
also found a FIFO issue. Would it make the most sense for me to submit a
V2 of this series with a few extra patches tacked on the end, or should
I wait for this series to be applied first? I can do it either way, I
just wasn't sure what would be preferred.

Thanks,
Doug

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

[Pavel Pisa]

Hello Doug

On Saturday 24 of August 2024 03:54:00 Doug Brown wrote:
> Thank you for reviewing all of my patches, Francisco.
>
> Now, all of these patches are reviewed but there are a few other issues
> we talked about here (dlc2len/len2dlc and issues with the flags), and I
> also found a FIFO issue. Would it make the most sense for me to submit a
> V2 of this series with a few extra patches tacked on the end, or should
> I wait for this series to be applied first? I can do it either way, I
> just wasn't sure what would be preferred.

I agree with both ways,

1) merging actual versing and then providing followup
   patches

2) updating and extending the series

I have little inclination to the second choice (2), because
you have already patch where you touch dlc2len/len2dlc issue
and making it without middle step would be more straightforward
and readable to me.

Anyway, I am the initiator of QEMU CAN subsystem as GSoC and studnets
mentor and coauthor but I have no commit right to the QEMU mainline.
So actual merge has to be realized by people with commit right.
Paolo Bonzini has provided help with CAN subsystem integration
and the committing.

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 2/5] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check

[Peter Maydell]

On Sat, 24 Aug 2024 at 02:55, Doug Brown <doug@schmorgal.com> wrote:
> Now, all of these patches are reviewed but there are a few other issues
> we talked about here (dlc2len/len2dlc and issues with the flags), and I
> also found a FIFO issue. Would it make the most sense for me to submit a
> V2 of this series with a few extra patches tacked on the end, or should
> I wait for this series to be applied first? I can do it either way, I
> just wasn't sure what would be preferred.

We're currently still in codefreeze for the upcoming 9.1 release,
so I would recommend sending a v2 with the extra patches. Nothing
except critical bugfixes is going to be applied upstream for
the next week or two.

(Since this is xilinx related I'm happy to pick it up in
a target-arm pullreq once we reopen for 9.2, unless anybody
wants to grab it some other way.)

thanks
-- PMM

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

[Doug Brown]

Hi Peter and Pavel,

On 8/25/2024 10:30 AM, Peter Maydell wrote:

> We're currently still in codefreeze for the upcoming 9.1 release,
> so I would recommend sending a v2 with the extra patches. Nothing
> except critical bugfixes is going to be applied upstream for
> the next week or two.

Thanks, that makes sense. Will do.

Doug