Hello Bernhard,
On Thursday 23 of July 2026 20:31:00 Bernhard Beschow wrote:
> Am 23. Juli 2026 10:51:05 UTC schrieb Pavel Pisa <[email protected]>:
> >Hello Bernhard and Philippe,
>
> Hi Pavel,
>
> >On Thursday 23 of July 2026 12:26:41 Bernhard Beschow wrote:
> >> Am 23. Juli 2026 08:20:34 UTC schrieb "Philippe Mathieu-Daudé"
> >
> ><[email protected]>:
> >> >On 23/7/26 09:00, Bernhard Beschow wrote:
> >> >> As indicated in the comments, the `mb[]` a.k.a. `mbs []` views should
> >> >> cover 0x400 (1024) bytes. However, the `mb[]` array covers four times
> >> >> the size since `sizeof(FlexcanRegsMessageBuffer)` returns the size in
> >> >> bytes instead of counting the number of uint32_t fields. This shifts
> >> >> the subsequent register offsets by 0xc00 which is not intended. Fix
> >> >> the size and thus the offsets by dropping the now unused `mb[]` view.
> >> >>
> >> >> Note that the different size changes the migration layout. Since the
> >> >> device model is new, no change in the version fields is needed.
> >> >
> >> >typedef struct FlexcanState {
> >> > ...
> >> > union {
> >> > FlexcanRegs regs;
> >> > uint32_t regs_raw[sizeof(FlexcanRegs) / 4];
> >> > };
> >> > ...
> >> >} FlexcanState;
> >> >
> >> >static const VMStateDescription vmstate_can = {
> >> > .name = TYPE_CAN_FLEXCAN,
> >> > .version_id = 1,
> >> > .minimum_version_id = 1,
> >> > .fields = (const VMStateField[]) {
> >> > ...
> >> > VMSTATE_UINT32_ARRAY(regs_raw, FlexcanState,
> >> > sizeof(FlexcanRegs) / 4),
> >> >
> >> > ^^^
> >> >
> >> >This "consider this structure as a casted array of u32" is very fragile
> >> >anyway, and will unlikely scale over time.
> >> >
> >> > ...
> >> > VMSTATE_END_OF_LIST(),
> >> > },
> >> >};
> >>
> >> Any suggestions what to do instead?
> >
> >I would suggest to keep the state as uint32_t array.
> >It is accessed by this and only by this size as the
> >FlexCAN MemoryRegionOps are defined. In the fact
> >I would like/plan in longer term, even to switch
> >the mbs array to be plain area of uint32_t in the future,
> >that is remove field
> >
> > FlexcanRegsMessageBuffer mbs[FLEXCAN_MAILBOX_COUNT];
> >
> >in FlexcanRegs.
> >
> >There should be only function which would return start
> >of given FlexcanRegsMessageBuffer based on index
> >argument and actual message buffers configuration.
> >It is required for CAN FD where message buffer
> >size is configurable. The raw/uint32_t area is kept
> >the same, but it is distributed to the indexed
> >buffers different way.
> >
> >So my preference is to move from structured view
> >in direction to the offsets in the words array.
> >Even the registers could be defined as offsets
> >to this array in the theory but as the overlay
> >structure for register accesses is used even
> >in the Linux kernel driver, I do not think that
> >this is necessary.
> >
> >But replacement of
> >
> > FlexcanRegsMessageBuffer mbs[FLEXCAN_MAILBOX_COUNT];
> >
> >with appropriate
> >
> > uint32_t mbs[FLEXCAN_MAILBOX_AREA_WORD_COUNT];
> >
> >and access functions would please me, because
> >it helps with future CAN FD support.
>
> Having CAN FD support would be nice indeed. Let's keep the code simple for
> now though until it gets implemented. Having just one way of doing things
> helps avoid traps such as those fixed in this series. We can change the
> code any time as we need it.
My main point is that attempt to implement VMStateDescription
some more structural way, i.e. mbs as array of corresponding
structures (which can be what has Philippe in mind), is counterproductive
from the longer term perspective, because the mbs role is the array
of words (uint32_t) which is interpretted according to other registers
in the FlexCAN3. In FlexCAN2, interpretation is fixed.
So storing whole area including control registers as array
of uint32_t matches reality quite well.
So I agree with keeping VMStateDescription unchanged.
Best wishes,
Pavel
Pavel Pisa
phone: +420 603531357
e-mail: [email protected]
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
