Add MSS CAN driver #337
Add MSS CAN driver #337
Conversation
haitomatic
commented
Jan 9, 2025
•
haitomatic
commented
- Add MSS CAN driver to support MSS CAN pheripheral
- Remove the unused canfd icicle config
haitomatic
force-pushed
the
SSRCDP-10527_CAN_MSS_support
branch
from
601f100 to
17d7f06
Compare
haitomatic
force-pushed
the
SSRCDP-10527_CAN_MSS_support
branch
from
17d7f06 to
2bb870d
Compare
| #define MPFS_CAN_ACR_AMR_IDE (1 << 2) /* IDE bit; 0: Check against ACR, 1: Don't care */ | ||
| #define MPFS_CAN_ACR_AMR_RTR (1 << 1) /* RTR bit; 0: Check against ACR, 1: Don't care */ | ||
|
|
||
| #endif /* __ARCH_RISCV_SRC_MPFS_HARDWARE_MPFS_CAN_H */ |
arch/risc-v/src/mpfs/mpfs_can.c
Outdated
| ****************************************************************************/ | ||
|
|
||
| #define print_uint32_t(prefix, val) do { \ | ||
| char binary_str[90]; /* 32 bits + "0b" + prefix + null terminator */ \ |
There was a problem hiding this comment.
Instead of 90, you could do
const char p_str[] = prefix; \
char binary_str[sizeof(p_str) + <needed size fo val>]; \
To make it safe... just a suggestion, I do see that 90 is enough in all uses below.
There was a problem hiding this comment.
thanks for the optimized suggestion!
| #ifdef CONFIG_NETDEV_CAN_FILTER_IOCTL | ||
| uint32_t can_id_masked = cf->can_id & CAN_ERR_MASK; | ||
|
|
||
| if (expect_true(!priv->filter.use_mask_filter |
There was a problem hiding this comment.
tinkering with expect is useless in general
arch/risc-v/src/mpfs/mpfs_can.c
Outdated
|
|
||
| /* Data (big endian) */ | ||
|
|
||
| for (i = 0; i < dlc; i++) |
There was a problem hiding this comment.
This is just unnecessarily complicated. The cf->data is always 32-bit aligned in nuttx can frames (intentionally). Also your pmsg->data_high and low are 32-bit aligned.
So you can just:
*(uint32_t *)&cf->data[0] = __builtin_bswap32(pmsg->data_high);
*(uint32_t *)&cf->data[4] = __builtin_bswap32(pmsg->data_low);
If in doubt, add a debugassertion for the alignment.....
There was a problem hiding this comment.
yes, this's much more elegant operation. i forgot about that. thanks!
arch/risc-v/src/mpfs/mpfs_can.c
Outdated
|
|
||
| if (!(pmsg->msg_ctrl & MPFS_CAN_TX_MSG_CTRL_CMD_RTR)) | ||
| { | ||
| for (i = 0; i < cf->can_dlc; i++) |
There was a problem hiding this comment.
This is again unnecessary complicated, imho. you can just copy&bswap both 32-bit words as in receive
There was a problem hiding this comment.
yes, that's much more elegant operation. i forgot about that. thanks!
arch/risc-v/src/mpfs/mpfs_can.c
Outdated
| canwarn("Failed to send CAN frame due to %s\n", | ||
| ret == CAN_INVALID_BUFFER ? "invalid buffer" : | ||
| ret == CAN_NO_MSG ? "no TX buffer available" : "unknown err"); | ||
| return CAN_OK; |
There was a problem hiding this comment.
Why return CAN_OK in case of failure?
There was a problem hiding this comment.
yes, err return handling of mpfs_transmit will be fixed. thanks!
arch/risc-v/src/mpfs/mpfs_can.c
Outdated
| { | ||
| /* Send the packet */ | ||
|
|
||
| mpfs_transmit(priv); |
There was a problem hiding this comment.
Should check the return value and return error (probably -EINVAL) in case of != CAN_OK
There was a problem hiding this comment.
yes, err return handling of mpfs_transmit will be fixed. thanks!
arch/risc-v/src/mpfs/mpfs_can.h
Outdated
| #endif | ||
|
|
||
| /**************************************************************************** | ||
| * Name: mpfs_fpga_canfd_init |
arch/risc-v/src/mpfs/mpfs_can.c
Outdated
| * bitrate - CAN controller bitrate | ||
| * | ||
| * Returned Value: | ||
| * On success, a pointer to the MPFS CANFD driver is |
| #ifdef CONFIG_MPFS_MSS_CAN0 | ||
| static mpfs_can_instance_t g_can0; | ||
|
|
||
| static uint8_t g_tx_pool0[(sizeof(struct can_frame) + TIMESTAMP_SIZE) * |
There was a problem hiding this comment.
It would be smart to allocate these as uint32_t buffers. Then you'll get can_frames aligned to 32-bit boundaries and can greatly simplify the copying of the frame data and other fields, as you know that they are all 32-bit aligned. added comments on this below
There was a problem hiding this comment.
hmm, since i think the CAN payload len dev->d_len is in bytes so it's just more natural to use uint8_t buffer. I took the reference from other CAN drivers also.
There was a problem hiding this comment.
for example ,
nuttx/arch/arm/src/imxrt/imxrt_flexcan.c
Line 314 in 67b95d0
There was a problem hiding this comment.
Sure, it was just a suggestion! You should anyhow align to 32-bits, in case you want to optimsize accessing of data. You can also use e.g. "aligned_data(sizeof(uint32_t))"
|
Looking good in general, I added a few questions and comments! |
haitomatic
force-pushed
the
SSRCDP-10527_CAN_MSS_support
branch
2 times, most recently
from
2aa2408 to
24407d4
Compare
haitomatic
force-pushed
the
SSRCDP-10527_CAN_MSS_support
branch
2 times, most recently
from
cc0ac79 to
2ed5e70
Compare
Remove unused icicle canfd config
haitomatic
force-pushed
the
SSRCDP-10527_CAN_MSS_support
branch
from
25e9f1c to
900f37f
Compare
