drivers: can: add tcan4x5x support #62033
Conversation
Add devicetree binding for the TI TCAN4x5x series of CAN controllers. These CAN controllers are based on the Bosch M_CAN IP and interfaced via a SPI bus. Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
Add driver for the TI TCAN4x5x series of CAN controllers. These CAN controllers are based on the Bosch M_CAN IP and interfaced via an SPI bus. Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
Add shield definition for the TI TCAN4550EVM, an evaluation module for the TI TCAN4x5x CAN controller series. Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
Add a compile test for the TI TCAN4x5x series CAN controller driver. Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
henrikbrixandersen
requested review from
nashif,
alexanderwachter,
erwango and
MaureenHelm
as code owners
zephyrbot
added
area: Process
platform: TI SimpleLink
zephyrbot
requested review from
avisconti,
cfriedt,
galak,
jfischer-no,
karstenkoenig,
kartben,
martinjaeger,
stephanosio,
str4t0m,
vaishnavachath and
vanti
henrikbrixandersen
removed
area: Process
platform: TI SimpleLink
|
|
||
| static int tcan4x5x_clear_mcan_mram(const struct device *dev, uint16_t offset, size_t len) | ||
| { | ||
| static const uint8_t buf[256] = {0}; |
There was a problem hiding this comment.
This looks like it wastes a lot of flash memory (if the compiler isn't able to optimize it away).
This function is only called when a filter is removed and once during driver init. Maybe we could just use 4 zero-bytes on the stack instead of these 256? This would be sufficient for removal of the rx filter, but it would lead to longer initialization because of significantly more SPI transfers.
Just an idea, not blocking.
There was a problem hiding this comment.
The tcan4x5x_clear_mcan_mram() takes up 88 bytes of flash. The buffer is stack allocated. Clearing the MRAM in 4 bytes chunks would result in 512 individual SPI transactions. A buffer size of 256 bytes was chosen since this is the largest TCAN4x5x SPI transfer size.
$ make rom_report |grep tcan4x5x_clear_mcan_mram
│ │ ├── tcan4x5x_clear_mcan_mram 88 0.07%
There was a problem hiding this comment.
As the buffer is a static variable, it is not counted as part of the function, but appears at the top of the rom_report:
Root 90116 100.00%
├── (hidden) 20108 22.31%
├── (no paths) 5696 6.32%
│ ...
│ ├── buf.0 256 0.28%
Anyway, don't have any hard feelings about this. We can keep it as is.
There was a problem hiding this comment.
I would prefer a compromise with a buffer of 16 or 32 bytes.
The mram clear is only executed once.
| const struct can_mcan_config *mcan_config = dev->config; | ||
| const struct tcan4x5x_config *tcan_config = mcan_config->custom; | ||
| size_t len32 = len / sizeof(uint32_t); | ||
| uint32_t src32[len32]; |
There was a problem hiding this comment.
This looks a bit dangerous to me.
Is there a point where we need to write more than one register at once (except the msgram clear).
There was a problem hiding this comment.
Yes, as you can see from the code this is also used for writing to the MRAM. Please elaborate on why you believe this is dangerous.
There was a problem hiding this comment.
Because the stack size is depending on a function parameter. A static size and a limit is saver in my opinion.
This PR adds devicetree binding and driver for the TI TCAN4x5x series of CAN controllers along with a shield definition for the TI TCAN4550EVM, an evaluation module for the TI TCAN4x5x CAN controller series. These CAN controllers are based on the Bosch M_CAN IP and interfaced via an SPI bus.