Re: [PATCH v1 0/6] CTU CAN FD core support - patchew report

[Pavel Pisa <pisa@cmp.felk.cvut.cz>]

Hello everybody,

On Tuesday 14 of July 2020 14:47:53 no-reply@patchew.org wrote:
> /tmp/qemu-test/src/hw/net/can/ctucan_pci.c: In function
> 'ctucan_pci_id_cra_io_read':
> /tmp/qemu-test/src/hw/net/can/ctucan_pci.c:105:20: error: format '%lx'
> expects argument of type 'long unsigned int', but argument 2 has type
> 'hwaddr' {aka 'long long unsigned int'} [-Werror=format=] 105 |    
> printf("addr=%lx,size=%x,tmp=%lx\n", addr, size, tmp);

excuse me, I have overlooked this printf used during debugging.
We fix that.

As for the following reported problem with formating, I am not sure
what to do. The hardware registers layout header files files are generated 
from CACTUS/IP-XACT XML fromat sources same as VHDL implementation
of registers accesses and fields.

The top level tool (pyXact_generator) is under our control (Ondrej Ille
is an author). Problem is, that tool output has been tuned to match
Linux kernel requirements, but QEMU has incompatible requirements
for indentation. Manual style correction in files is not a right
way to go. Adding switch for QEMU style to pyXact_generator
is possible but then header files intended for the Linux kernel
cannot be compared with corresponding QEMU ones and generally,
configurable setup and multiple options to generate files
would lead to mess. We take directly header files from main
CTU CAN FD IP core repository where match between sources and generated
files is checked as continuous integration step.


On Tuesday 14 of July 2020 14:48:50 no-reply@patchew.org wrote:
> Patchew URL:
> https://patchew.org/QEMU/cover.1594725647.git.pisa@cmp.felk.cvut.cz/
...
> === OUTPUT BEGIN ===
> 1/6 Checking commit 28bfb20d4aa8 (net/can: Initial host SocketCan support
> for CAN FD.) 2/6 Checking commit d9207e106794 (hw/net/can: sja1000 ignore
> CAN FD frames) 3/6 Checking commit 733ab67ddcc2 (net/can: Add can_dlc2len
> and can_len2dlc for CAN FD.) 4/6 Checking commit dc95d778bb88
> (hw/net/can/ctucafd: Add CTU CAN FD core register definitions.) WARNING:
> added, moved or deleted file(s), does MAINTAINERS need updating? #30:
> new file mode 100644
>
> WARNING: Block comments use a leading /* on a separate line
> #36: FILE: hw/net/can/ctu_can_fd_frame.h:2:
> +/*************************************************************************
>******
>
> WARNING: architecture specific defines should be avoided
> #67: FILE: hw/net/can/ctu_can_fd_frame.h:33:
> +#ifndef __CTU_CAN_FD_CAN_FD_FRAME_FORMAT__
>
> ERROR: code indent should never use tabs
> #72: FILE: hw/net/can/ctu_can_fd_frame.h:38:
> +^ICTU_CAN_FD_FRAME_FORM_W        = 0x0,$
>
> ERROR: code indent should never use tabs
> #73: FILE: hw/net/can/ctu_can_fd_frame.h:39:
> +^ICTU_CAN_FD_IDENTIFIER_W        = 0x4,$

> ERROR: code indent should never use tabs
> #221: FILE: hw/net/can/ctu_can_fd_frame.h:187:
> +^I} s;$
>
> WARNING: Block comments use a leading /* on a separate line
> #232: FILE: hw/net/can/ctu_can_fd_regs.h:2:
> +/*************************************************************************
>******
>
> WARNING: architecture specific defines should be avoided
> #263: FILE: hw/net/can/ctu_can_fd_regs.h:33:
> +#ifndef __CTU_CAN_FD_CAN_FD_REGISTER_MAP__
>
> ERROR: code indent should never use tabs
> #268: FILE: hw/net/can/ctu_can_fd_regs.h:38:
> +^ICTU_CAN_FD_DEVICE_ID             = 0x0,$
>
> ERROR: code indent should never use tabs
> #269: FILE: hw/net/can/ctu_can_fd_regs.h:39:
> +^ICTU_CAN_FD_VERSION               = 0x2,$
>
> ERROR: code indent should never use tabs
> #270: FILE: hw/net/can/ctu_can_fd_regs.h:40:
> +^ICTU_CAN_FD_MODE                  = 0x4,$

As for the functionality, the solution has been tested only for little-endian
guests and host systems for now. I am not fully sure how things map on 
different variants of big-endian systems and how well they would match
reality. Generally, the Linux driver is written such way that it detects
switched byte order and should adapt for it automatically.

I can try some MIPS based system, if the check result is important.
But it would worth to have even real HW to compare real programmed
PCIe card behavior and I do not pose Linux capable big endian systems.
I have some safety based Cortex-R big endian ARMS there, but they do not
have PCIe and QEMU support.

Thanks for review and suggestions in advance,

                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://dce.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/