Dear Matyas Bobek and Bernhard Beschow,
On Monday 15 of December 2025 21:03:09 Matyáš Bobek wrote:
> This series adds emulation of the FlexCAN CAN controller, version 2,
> found in NXP i.MX6 series SoCs. The controller is integrated into
> fsl-imx6 and the Sabrelite ARM board.
First, thanks a lot to Matyas Bobek for finding the courage
and sending the FlexCAN series, finally, after keeping it updated
log time on his flexcan-series-XXX branches at his personal
QEMU development repository
https://gitlab.fel.cvut.cz/bobekmat/qemu-flexcan/-/branches
Bernhard Beschow, thanks for expressing interrest in the project.
Your intererst helped Matyas Bobek to collect courage to send.
I have found that you have invested a lot in the CAN on
your imx8mp-flexcan and can-cleanup branches, thanks again
https://github.com/shentok/qemu/branches
It is shame that long delay in sending of patches has
lead to some divergence of the effort.
I have gone through your changes and would be happy if the
effort can be joined and integrated into mainline.
I would prefer to help Matyas Bobek's series to be updated
in state that it passes review and then the FlexCAN
can be moved forward by you and others.
In long term, the extension to support FlexCAN3 would
be very usesfull. But that that is work for other thesis,
GSoC and or company funded project or developers.
I would be happy to provide my knowledge as the time
allows or look for students, etc.
As for the series and your (Bernhard Beschow) changes:
I have seen that you suggest some reordering of some
functions in the hw/net/can/flexcan.c file. If you think
that it is the better, more readable order for QEMU developers,
I would suggest and have plea to Matyas Bobek to proceed
with reorder. Same with tracepoints and debug prints
which could be updated into state that your or others
follow-up patches would not cause massive code movement
which complicates tracking and reading of the changes.
On the other hand, I have some arguments against unification
of memory FlexCAN access function and inlining register accesses
into them. We have already discused with Matyas Bobek that
for FlexCAN3 and other future changes it would worth to
separate registers from memory part etc. So I would kept
this separation. Making as much as possible of the core
opaque for its external use is right from my view point
on the other hand.
As for the CAN core changes, again there are some which
I see as good moves, some cleanup of long term unused original
structures which have been planned for another integration
into QEMU before simplification to pass review etc. On the other
hand, I would keep client state without const and with destructors
etc. Again, actual code is somehow usable in its actual form,
but from the long term perspective, I see the need for
back-pressure propagation, emulation of the highest priority
message (the lowest ID) exchange the first, etc. and I have some
plans for that. I do not think that CAN emulation is and will
be some real performance bottleneck in QEMU use for embedded
systems development so I would like to keep there space
for future more precise emulation.
Same with reset on the chip core level, I think that its
redundant call does not cause any performance problems,
but I would be happy if the controller codes are reusable
for wide scenarios. I have written and used LinCAN with
such controllers at time of ISA bus on PC. I would be happy if
we have mechanism how to map them on SoCs with FPGAs.
Unfortunately this valid and very usesfull feature
(
for example for our RTEMS effort on Zynq
https://docs.rtems.org/docs/main/bsp-howto/can.html
and ideally even on PolarFire (as time allows) where even NuttX
can be tested and CI run
https://github.com/apache/nuttx/tree/master/drivers/can
)
hit concrete wall in May, without any suggestion how to
make that needed use of QEMU for CI acceptable.
But our SJA1000 code is already used by Espressif in their
QEMU fork
https://github.com/espressif/qemu/tree/esp-develop/hw/net/can
so there is more proven use out of PCI based cards. CTU CAN FD
is used mostly on FPGAs but here are MCUs with it so again,
even if the usability of mainline QEMU for FPGA development
would stay blocked, there are standard, hopefully non problematic,
regular machine code initiated uses of the CAN controllers
which are part of QEMU.
So I would be happy if we can thought about that wider use
to check that it would not be more problematic in future
if some code is optimized.
On the other hand, it is right that even in esp32_twai.c
case, the integration is based on RESETTABLE_CLASS
and esp32_twai_reset() calls can_sja_hardware_reset()
explicitly. So can_sja_hardware_reset() during can_sja_init()
is not strictly necessary.
Back to sabrelite FlexCAN support series.
In general I agree with the patch series and I have
consulted and reviewed it multiple times.
So it can be considered to be approved by me
that it is functionally OK as well as it respects
copyright requirements etc. I add my
Signed-off-by: Pavel Pisa <pisa@fel.cvut.cz>
As for individual formatting and may be some debug prints,
I would allow it to go in in its actual form and then reduce
these latter that we have state with more, may it be even
abundant, debug in mainline recorded. But I expect that
there could be more request for style and details from
more experienced QEMU developers.
There is one unresolved patch check report about
DEVICE_NATIVE_ENDIAN
+static const struct MemoryRegionOps flexcan_ops = {
+ .read = flexcan_mem_read,
+ .write = flexcan_mem_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ .unaligned = true,
+ .accepts = flexcan_mem_accepts
+ },
+ .impl = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ .unaligned = false
+ },
+};
But I do not know what I can suggest there. The device is
infernally accessed by 32-bits words and should be
mapped in native format because same core is used
on little-endian and may it be even bi-endian ARMs[*1]
and for sure on big-endian PowerPCs. We believe that
native endianness with host is the best option in this
case. Extending .valid.max_access_size to 8 is right
and probably require for 64-bit targets as I understand
from your patches.
[*1] as I have used bi-endian ARMs they mapped peripherals
often native way on 32-bit entities. So again, fixed
DEVICE_BIG_ENDIAN or DEVICE_LITTLE_ENDIAN is incorrect
in such case.
Best wishes,
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://control.fel.cvut.cz/
personal: http://cmp.felk.cvut.cz/~pisa
social: https://social.kernel.org/ppisa
projects: https://www.openhub.net/accounts/ppisa
CAN related:http://canbus.pages.fel.cvut.cz/
RISC-V education: https://comparch.edu.cvut.cz/
Open Technologies Research Education and Exchange Services
https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home