Re: [QEMU][PATCH 2/5] hw/net/can: Introduce Xilinx Versal CANFD controller

[Peter Maydell]

On Sat, 10 Sept 2022 at 07:13, Vikram Garhwal <vikram.garhwal@amd.com> wrote:
>
> The Xilinx Versal CANFD controller is developed based on SocketCAN, QEMU CAN 
> bus
> implementation. Bus connection and socketCAN connection for each CAN module
> can be set through command lines.
>
> Example for using connecting CANFD0 and CANFD1 to same bus:
>     -machine xlnx-versal-virt
>     -object can-bus,id=canbus
>     -machine canbus0=canbus
>     -machine canbus1=canbus
>
> To create virtual CAN on the host machine, please check the QEMU CAN docs:
> https://github.com/qemu/qemu/blob/master/docs/can.txt

That link is a 404. You could just give the relative path to the
docs in the repo, which is docs/system/devices/can.rst

For the machine specifics, you should include (either in the patch 4
where you add this to the xlnx-versal-virt board, or in a separate patch
if it seems too big) updates to docs/system/arm/xlnx-versal-virt.rst
which document the new functionality, including, if it's useful to users,
some documentation of how to use it.


> +/* To avoid the build issues on Windows machines. */
> +#undef ERROR

What ?

> +static void canfd_config_mode(XlnxVersalCANFDState *s)
> +{
> +    register_reset(&s->reg_info[R_ERROR_COUNTER_REGISTER]);
> +    register_reset(&s->reg_info[R_ERROR_STATUS_REGISTER]);
> +    register_reset(&s->reg_info[R_STATUS_REGISTER]);
> +
> +    /* Put XlnxVersalCANFDState in configuration mode. */
> +    ARRAY_FIELD_DP32(s->regs, STATUS_REGISTER, CONFIG, 1);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, WKUP, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, SLP, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, BSOFF, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, ERROR, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXFOFLW, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXFOFLW_1, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXOK, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, TXOK, 0);
> +    ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, ARBLST, 0);
> +
> +    /* Clear the time stamp. */
> +    ptimer_transaction_begin(s->canfd_timer);
> +    ptimer_set_count(s->canfd_timer, 0);
> +    ptimer_transaction_commit(s->canfd_timer);
> +
> +    canfd_update_irq(s);
> +}
> +

A lot of this looks like it's just copy-and-pasted code from
the existing hw/net/can/xlnx-zynqmp-can.c. Is this just an
updated/extra-features version of that device? Is there some
way we can share the code rather than duplicating 2000-odd lines ?

> +#ifndef HW_CANFD_XILINX_H
> +#define HW_CANFD_XILINX_H
> +
> +#include "hw/register.h"
> +#include "hw/ptimer.h"
> +#include "net/can_emu.h"
> +#include "hw/qdev-clock.h"
> +
> +#define TYPE_XILINX_CANFD "xlnx.versal-canfd"

Should this be a dot or a comma? The codebase has examples of
both for xlnx devices :-(

> +
> +#define XILINX_CANFD(obj) \
> +     OBJECT_CHECK(XlnxVersalCANFDState, (obj), TYPE_XILINX_CANFD)

Please use OBJECT_DECLARE_SIMPLE_TYPE() rather than defining the
cast macro by hand.

> +
> +#define NUM_REGS_PER_MSG_SPACE 18
> +#define MAX_NUM_RX             64
> +#define CANFD_TIMER_MAX        0XFFFFUL

Don't use capital X in the 0x hex prefix, please.

> +#define CANFD_DEFAULT_CLOCK    (24 * 1000 * 1000)
> +
> +/* 0x4144/4 + 1 + (64 - 1) * 18 + 1. */

This comment isn't very informative. The #define itself is much
better because it uses symbolic constants.

What is the magic number 0x4144. It should either be defined via
some kind of symbolic constant, or if that's not possible at least
explained in a comment.

> +#define XLNX_VERSAL_CANFD_R_MAX (0x4144 / 4  + \
> +                    ((MAX_NUM_RX - 1) * NUM_REGS_PER_MSG_SPACE) + 1)

thanks
-- PMM