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
>