mbox: mbox_ti_secproxy: Driver optimization and bugfixes#100642
mbox: mbox_ti_secproxy: Driver optimization and bugfixes#100642aescolar merged 7 commits intozephyrproject-rtos:mainfrom
Conversation
Kronosblaster
force-pushed
the
secproxy_fix
branch
from
49b5791 to
6e6e026
Compare
Kronosblaster
force-pushed
the
secproxy_fix
branch
4 times, most recently
from
0677368 to
f55a1d1
Compare
Kronosblaster
force-pushed
the
secproxy_fix
branch
from
f55a1d1 to
b2986ef
Compare
Kronosblaster
force-pushed
the
secproxy_fix
branch
3 times, most recently
from
e0d77e2 to
f40c63b
Compare
Kronosblaster
force-pushed
the
secproxy_fix
branch
6 times, most recently
from
c9a468e to
3d9df60
Compare
zephyrbot
added
area: Devicetree Bindings
platform: TI K3
|
do we still need #101338? seems like it's included here as well |
Yes. This will be separated. Temporary WIP error. Drafted the PR till the update is complete. |
|
I'm not going to review this further until the commit messages are fixed, I've asked for fixing the commit messages before. Github doesn't have a good way to comment directly on the commit messages themselves but for all messages you need to do the following. Each must give the "why" a patch is needed, not just a list of what the change does, I can see what the change does just by looking at the patch diff. No lists either, this is usually a sign you are doing too many things in one commit. Each commit must be logically minimal but fully complete and stand-alone. |
Sorry for the effort. Will be updated asap |
| } | ||
|
|
||
| /* Check the message queue before sending/receiving data */ | ||
| int timeout_ms = SEC_PROXY_TIMEOUT_US; |
There was a problem hiding this comment.
If what you want to have no delay before timing out, why not flip the below loop to check if timeout has passed first. Then you can set SEC_PROXY_TIMEOUT_US in the case you want this to timeout instantly.
Your commit message states that "retry logic should be handled by the calling layer" but I don't see any retry logic anywhere in the calling path of this function nor in any example using mailbox.
There was a problem hiding this comment.
This check shouldn't be done in this layer at all. As a peripheral driver it should just return the appropriate error message and be done with it. It is much cleaner to return these error messages. Additionally k_busy_wait shouldn't be called pre-kernel where this driver should be functional. Adding a pre-kernel check could be done but that is adding additional complexity where it is not required.
You're right about the commit message and I have updated the commit message to clarify that retry messages can be optionally implemented if required. This layer is only used by TISCI. The retry logic is an improvement for the TISCI driver and will be done in another patch if required. Not currently pushing that as this PR is a dependency for a few other PRs which have been blocked for a while.
Thanks
There was a problem hiding this comment.
It's easy to say it shouldn't be done in this layer to make this layer less complex, but all you would be doing is pushing that complexity higher in the stack, and potentiality into every user of this mailbox. Meaning more complexity in more spots..
Since that higher layer will likely only be TI-SCI, I won't hold this series up on this, but with the understanding that should this need to be done you will need to be the one to add the retry logic to TI-SCI layer.
Remove busy wait from secproxy_verify_thread() to return immediately with error status. If mailbox is full, waiting for message processing is unnecessary and blocks execution until timeout, reporting a timeout error message reducing debuggability. The mailbox driver should only return the appropriate error code. Any required retry logic can be implemented in the caller layer. Signed-off-by: Dave Joseph <d-joseph@ti.com>
Forward actual error codes from secproxy_verify_thread() instead of masking all errors as -EBUSY. Now returns -EINVAL, -EBUSY, or -ENODATA from secproxy_mailbox_send() to aid debugging. Signed-off-by: Dave Joseph <d-joseph@ti.com>
Remove duplicate error validation in ISR and send path that is already performed by secproxy_verify_thread(). Remove error mask checks, message queue fullness checks, and unused SECPROXY_MAILBOX_NUM_MSGS macro. Signed-off-by: Dave Joseph <d-joseph@ti.com>
Add interrupt-names property to binding to enable per-channel interrupt lookup via DT_INST_IRQ_BY_NAME(). Each channel's interrupt is identified by name: "rx_0", "rx_1", "rx_2", etc. Signed-off-by: Dave Joseph <d-joseph@ti.com>
Split interrupt configuration from am64x_r5.dtsi into individual R5F core files (am64x_r5f0_0.dtsi, am64x_r5f0_1.dtsi, am64x_r5f1_0.dtsi, am64x_r5f1_1.dtsi) as each core uses different mailbox channels and VIM interrupt line mappings. Signed-off-by: Dave Joseph <d-joseph@ti.com>
Replace single shared interrupt with per-channel interrupt support. Change .irq to .interrupts[] array in config, use LISTIFY macros for ISR generation and connection, add channel_enable[] array for per-channel state tracking. Each rx channel now has independent ISR. Signed-off-by: Dave Joseph <d-joseph@ti.com>
Add secproxy_mailbox_flush_thread() to clear stale messages from rx thread when enabling a channel. Read and discard all pending messages to ensure clean thread state and prevent errors from previous unclean state. Signed-off-by: Dave Joseph <d-joseph@ti.com>
|
6 participants
Improves the TI secure proxy mailbox driver with better error handling, per-channel interrupt support, and thread cleanup.
1: Fix thread verification
2: Support per-channel interrupts
3: Add thread flush on channel enable
4: Add SPDX License Identifier