drivers: can: Add Renesas R-Car RSCANFD driver#109782
Conversation
zephyrbot
added
area: Devicetree Binding
zephyrbot
requested review from
JarmouniA,
alexanderwachter,
henrikbrixandersen,
lorc,
martinjaeger,
str4t0m,
william-tang914 and
xakep-amatop
|
Reassigned to self as CAN subsystem maintainer. |
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
5 times, most recently
from
7e289a3 to
5b2ca97
Compare
|
The driver has been tested on the upcoming Renesas X5H board (the PRs adding this board support to Zephyr are still in review). The CAN API tests are passing : |
RICCIARDI-Adrien
changed the title
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
from
5b2ca97 to
de3695f
Compare
| can_rcar_rscanfd_configure_timing_data(dev, &timing); | ||
| #endif | ||
|
|
||
| config->configure_irq(); |
There was a problem hiding this comment.
Could the IRQ be connected/enabled only after the instance data has been fully
initialized and after stale TX/RX/error flags have been cleared?
Right now configure_irq() is called before the mutexes/semaphore are
initialized and before the channel is registered. Also, the TX path in the ISR
unconditionally calls data->tx_callback(). The CAN core guarantees a non-NULL
callback for a normal can_send() path, but a stale/pending TX interrupt before
the first send would still leave data->tx_callback == NULL.
At minimum I think the driver should initialize the software state first and
clear any pending hardware interrupt flags before enabling the IRQ.
There was a problem hiding this comment.
This is a relevant remark. As I could manage to avoid any global controller interrupt, I updated the code to enable a channel interrupt only when the channel has been started, and to disable the channel interrupt when the channel is stopped. This way, a channel ignores interrupts while it is not started, this could save some cycles.
There was a problem hiding this comment.
Thanks, moving IRQ_CONNECT after the software state initialization addresses the
original initialization-order concern.
One remaining question: should the IRQ controller line really be used as the
runtime channel start/stop gate?
I would normally expect IRQ_CONNECT/irq_enable() to be part of the device
initialization path, after stale peripheral status has been cleared. Then
start()/stop() would control interrupt generation through the RSCANFD interrupt
enable bits in the device registers.
Using irq_enable()/irq_disable() in start()/stop() makes the IRQ controller line
part of the CAN channel runtime state. Is there a reason this cannot be handled
only through the controller's own interrupt mask/enable registers instead?
There was a problem hiding this comment.
That's correct, enabling or disabling the IRQ line (or even modifying the IP interrupt registers) should even not be needed, because the CAN IP is in halt mode when stopped. Thus, no interrupt will be serviced because nothing can be transmitted (there is also a software check in can_rcar_rscanfd_send()) or received. I reverted these changes back.
| if (data->rx_callback[i] == NULL) { | ||
| data->rx_callback_user_data[i] = user_data; | ||
| data->filter[i] = *filter; | ||
| data->rx_callback[i] = callback; |
There was a problem hiding this comment.
rx_mutex does not protect the RX filter table from the RX ISR. The ISR
traverses rx_callback[], rx_callback_user_data[] and filter[] without
taking that mutex, so a filter can be removed or reused while the ISR is in the
middle of matching it.
For example, remove_rx_filter() can clear rx_callback[i] after the ISR has
already observed it as non-NULL, and a later add_rx_filter() can reuse the same
slot before the ISR reads filter[i] / rx_callback_user_data[i] or invokes the
callback. This can result in callbacks after removal, or in a mixed old/new
filter entry being used.
Should filter table updates be protected with IRQ masking or an ISR-safe lock,
or should the driver document that callbacks may still run after removal and
that slots must not be immediately reused?
There was a problem hiding this comment.
Another very good catch ! rx_mutex is a relic that should have been gone. I used the inst_mutex instead to protect the add_rx_filter() and remove_rx_filter() from concurrent accesses from different threads, and also masked the IRQs when each of these functions is accessing the variables shared with the IRQ handler.
There was a problem hiding this comment.
Thanks, this addresses part of the original issue, but I am still not sure this
is sufficient.
add_rx_filter() and remove_rx_filter() now unconditionally re-enable the channel
IRQ after updating the filter table. This can enable the IRQ while the channel is
stopped, which seems to break the new invariant that channel IRQs are enabled
only from start() and disabled from stop().
There is also a possible SMP aspect here. Nothing seems to restrict this driver
to a single-core Cortex-R configuration only; the same IP could be used from
application cores on an SMP system. In that case, disabling the IRQ line while
holding inst_mutex does not obviously serialize against an ISR that is already
running on another CPU, and the ISR does not take inst_mutex.
Would it be better to use an ISR-safe synchronization primitive for the filter
table, or at least preserve the previous IRQ enable state and only re-enable the
IRQ when the channel was already started?
There was a problem hiding this comment.
Your concerns are legitimate. Let's indeed make the behavior more correct and robust ! I have updated the code.
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
from
de3695f to
e631339
Compare
|
@xakep-amatop Thanks a lot for your thorough and very efficient reviews ! |
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
from
e631339 to
a3afa03
Compare
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
from
a3afa03 to
1ed1a26
Compare
The RSCANFD is capable of CAN and CAN FD communications. It features 8 channels per controller. The controller itself is referred as "global", and there is a specific binding for the channels. Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
from
1ed1a26 to
69ac37d
Compare
|
This PR is fully complete now that the X5H board support has been merged. I could add all the required device tree nodes. The |
Samples are not tests. The correct way to verify this is to use the test suites under |
Sure, the test suites have also been run. The result of the API test is identical to the one in a previous comment of this PR : #109782 (comment). |
henrikbrixandersen
left a comment
henrikbrixandersen
left a comment
There was a problem hiding this comment.
Please also add a build-only test to tests/drivers/build_all/can/.
| # CAN-related configurations | ||
| CONFIG_CAN=y | ||
| CONFIG_CAN_FD_MODE=y |
There was a problem hiding this comment.
These should not be set in a board default configuration. Only options needed for booting the board with a console should be there (e.g. GPIO should not be there either, unless the board needs a given GPIO (hog) to be set for booting, same for I2C).
There was a problem hiding this comment.
OK, fixed for the CAN part. Other unwanted configuration options will require a new PR to be removed.
| zephyr,sram = &sram0; | ||
| zephyr,console = &hscif1; | ||
| zephyr,shell-uart = &hscif1; | ||
| zephyr,canbus = &can0; |
There was a problem hiding this comment.
You need to add can to boards/renesas/rcar_ironhide_x5h/rcar_ironhide_x5h_r8a78000_r52.yaml as well for this board to be picked up by twister for any CAN tests.
There was a problem hiding this comment.
Thanks for spotting this, it's now fixed.
| config CAN_RCAR_MAX_FILTERS | ||
| int "Maximum number of concurrent active filters" | ||
| depends on CAN_RCAR | ||
| depends on CAN_RCAR || CAN_RCAR_RSCANFD |
There was a problem hiding this comment.
I would suggest introducing a CAN_RCAR_RSCANFD_MAX_FILTERS instead to keep the namespaces.
| static inline int can_rcar_rscanfd_busy_wait(mem_addr_t reg, uint32_t bit_mask, bool expect_one) | ||
| { | ||
| const int MAX_RETRIES = 256; /* Don't wait for too long */ | ||
| int i; | ||
| uint32_t val; | ||
|
|
||
| for (i = 0; i < MAX_RETRIES; i++) { | ||
| val = sys_read32(reg) & bit_mask; | ||
| if (!expect_one) { | ||
| val = !val; | ||
| } | ||
| if (val) { | ||
| return 0; | ||
| } | ||
|
|
||
| k_sleep(K_USEC(10)); | ||
| } | ||
|
|
||
| LOG_DBG("Busy wait timed out. reg=0x%08lX, bit_mask=%08X, expect_one=%d", | ||
| reg, bit_mask, expect_one); | ||
| return -EAGAIN; | ||
| } |
There was a problem hiding this comment.
Please investigate using the WAIT_FOR() macro from include/zephyr/sys/util.h here instead.
| } | ||
| state_mask <<= RSCANFD_CFDCFCCN_CFE_SHIFT; | ||
|
|
||
| /* The first Common FIFO dedicated to the channel is used for transmission */ |
There was a problem hiding this comment.
So this uses a FIFO for TX? This is usually not good, as CAN frames should not be transmitted in FIFO order, but rather in the priority order given by the CAN IDs of the frames. Please clarify if this FIFO implements priority ordering.
RX path should be a FIFO, if possible.
| if (frame->flags & CAN_FRAME_ESI) { | ||
| val |= RSCANFD_CFDCFFDCSTSBN_CFESI; | ||
| } |
There was a problem hiding this comment.
Software should not be able to set this bit on transmitted frames; that's up to the controller hardware.
There was a problem hiding this comment.
The controller hardware automatically overwrites this bit if it is in the error passive state to tell that the node is in error passive state. However, when the node is not in error passive state, the user defined bit value is transmitted (a value of 0 means that the controller is in error active state).
I'm then forcing the bit to 0, so the controller takes indeed care of setting it when the node becomes error passive.
| /* Enable the transceiver delay compensation for high speeds */ | ||
| bit_rate = (CAN_RCAR_RSCANFD_MODULE_CLOCK_RATE / timing->prescaler) / | ||
| (1 + timing->prop_seg + timing->phase_seg1 + timing->phase_seg2); | ||
| if (bit_rate >= 5000000) { | ||
| val = can_rcar_rscanfd_read(dev, base_offset + RSCANFD_CFDCNFDCFG); | ||
| /* Use the measured compensation + an offset of 0 */ | ||
| val &= ~RSCANFD_CFDCNFDCFG_TDCOC; | ||
| val |= RSCANFD_CFDCNFDCFG_TDCE; | ||
| can_rcar_rscanfd_write(dev, base_offset + RSCANFD_CFDCNFDCFG, val); |
There was a problem hiding this comment.
Please look into using CAN_CALC_TDCO().
| while (1) { | ||
| val = can_rcar_rscanfd_read(dev, base_offset); | ||
| if (val & bit_mask) { | ||
| val &= ~bit_mask; | ||
| can_rcar_rscanfd_write(dev, base_offset, val); | ||
| } else { | ||
| break; | ||
| } | ||
| } |
| return 0; | ||
| } | ||
|
|
||
| static DEVICE_API(can, can_rcar_rscanfd_driver_api) = { |
There was a problem hiding this comment.
How about CAN bus recovery? How does this core handle that?
| BUILD_ASSERT(CONFIG_CLOCK_CONTROL_INIT_PRIORITY < CONFIG_KERNEL_INIT_PRIORITY_DEVICE, | ||
| "The clocks must be initialized before the CAN controller."); | ||
| BUILD_ASSERT(CONFIG_KERNEL_INIT_PRIORITY_DEVICE < CONFIG_CAN_INIT_PRIORITY, | ||
| "The CAN controller must be initialized before its channels."); |
There was a problem hiding this comment.
Isn't this automatically ensured based on devicetree ordinals?
There was a problem hiding this comment.
It is, I get the Device initialization priority validation failed, the sequence of initialization calls does not match the devicetree dependencies. message if I set the CONFIG_CAN_INIT_PRIORITY thread priority lower than the CONFIG_KERNEL_INIT_PRIORITY_DEVICE thread priority one.
The asserts just make the reason clearer, but I can remove the asserts if you prefer.
Done ! |
The RSCANFD is capable of CAN and CAN FD communications. It features 8 channels per controller. Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
The SoC features two RSCANFD controllers with 8 channels each. Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
Define all the supported CAN pins, but enable only the CAN controllers that are available on the board. Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
Add the RSCANFD driver. Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
RICCIARDI-Adrien
force-pushed
the
aricciardi/rcar_add_rscanfd_driver
branch
from
69ac37d to
d35e7d8
Compare
zephyrbot
added
the
area: Tests
|
I've pushed a first batch of changes to see how the CI will behave. I'm working on the remaining changes. |
|
5 participants
The RSCANFD CAN IP is found in Gen 4 and Gen 5 boards.
It features 2 controllers with 8 independant channels each.
All channels are classic CAN and CAN FD capable.