Re: [PATCH v11 1/4] hw/net/can: Introduce Xilinx ZynqMP CAN controller

[Peter Maydell]

On Wed, 14 Oct 2020 at 07:04, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> The Xilinx ZynqMP CAN 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 single CAN:
>     -object can-bus,id=canbus0 \
>     -machine xlnx-zcu102.canbus0=canbus0 \
>     -object can-host-socketcan,id=socketcan0,if=vcan0,canbus=canbus0
>
> Example for connecting both CAN to same virtual CAN on host machine:
>     -object can-bus,id=canbus0 -object can-bus,id=canbus1 \
>     -machine xlnx-zcu102.canbus0=canbus0 \
>     -machine xlnx-zcu102.canbus1=canbus1 \
>     -object can-host-socketcan,id=socketcan0,if=vcan0,canbus=canbus0 \
>     -object can-host-socketcan,id=socketcan1,if=vcan0,canbus=canbus1
>
> To create virtual CAN on the host machine, please check the QEMU CAN docs:
> https://github.com/qemu/qemu/blob/master/docs/can.txt
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>


> +#define MAX_CAN_CTRLS      2
> +#define XLNX_ZYNQMP_CAN_R_MAX     (0x84 / 4)
> +#define MAILBOX_CAPACITY   64
> +#define CAN_TIMER_MAX  0XFFFFUL

"0x" is more usual.


> +    if (ARRAY_FIELD_EX32(s->regs, SOFTWARE_RESET_REGISTER, SRST)) {
> +        trace_xlnx_can_can_receive("Controller is in reset.\n");
> +        return false;
> +    }

> --- /dev/null
> +++ b/hw/net/can/trace-events
> @@ -0,0 +1,9 @@
> +# xlnx-zynqmp-can.c
> +xlnx_can_transfer_fifo(const char *message) "%s"
> +xlnx_can_srr_pre_write(const char *message) "%s"
> +xlnx_can_update_rx_fifo(const char *message) "%s"
> +xlnx_can_rxfifo_pre_read(const char *message) "%s"
> +xlnx_can_tx_post_write(const char *message) "%s"
> +xlnx_can_can_receive(const char *message) "%s"
> +xlnx_can_receive(const char *message) "%s"
> +xlnx_can_realize(const char *message) "%s"

This is not the usual way to do tracepoints. Generally rather
than having one trace point which gets passed an opaque string,
you should have trace points for each interesting event,
which have an event-specific format string that prints out
the useful information. Here's an example from a UART model:
 pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x
read_count %d returning %d"
Notice that it's specific to one event (there's only one
place in the code that calls that tracepoint), it mostly
relies on the name of the trace event itself to give the
user context, and it uses the format string to provide some
information about the internal state of the device (not always
relevant for all trace events -- sometimes you do just have an
event).

That way users of tracepoints can enable, for instance,
all the 'xnlx_can_rxfifo*' tracepoints if they want all
the logging, or only 'xlnx_can_rxfifo_full' if they only
care to log the "fifo filled up" event but not others.

thanks
-- PMM