Add MSS CAN driver #337
Add MSS CAN driver #337
Conversation
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
| @@ -0,0 +1,141 @@ | |||
| /**************************************************************************** | |||
| * arch/risc-v/src/mpfs/hardware/mpfs_can.h | |||
| * | |||
| #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 */ |
| ****************************************************************************/ | ||
|
|
||
| #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.
| #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
|
|
||
| /* 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.....
| * queue is not full. | ||
| */ | ||
|
|
||
| priv->dev.d_buf = (uint8_t *)priv->txdesc; |
There was a problem hiding this comment.
Does this actually make sense? looks like some useless copy-paste from somewhere
|
|
||
| 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
| 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?
| { | ||
| /* 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
| #endif | ||
|
|
||
| /**************************************************************************** | ||
| * Name: mpfs_fpga_canfd_init |
| * 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
|
|
||
| /* Ignore the notification if the interface is not yet up */ | ||
|
|
||
| if (priv->bifup) |
There was a problem hiding this comment.
Is there a race between this and ifup/ifdown? Should actually this and also mpfs_can_send_message_ready be inside the net_lock? I am not sure, just checking
|
Looking good in general, I added a few questions and comments! |
Add MSS CAN driver to support MSS CAN pheripheral