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

[Vikram Garhwal <fnu.vikram@xilinx.com>]

On Tue, Oct 20, 2020 at 11:53:25AM +0100, Peter Maydell wrote:
> 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.
I will correct this.
>
>
> > +    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.
>
Hi Peter,
Thank you so much for explaining this in detail. I will create distinct
tracepoints for each event in next version. I will prepare v12 and send it soon.
> thanks
> -- PMM