-
Notifications
You must be signed in to change notification settings - Fork 8k
NXP Add SCMI P2A workflow and BBNSM protocol #93305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
NXP Add SCMI P2A workflow and BBNSM protocol #93305
Conversation
8f29172
to
e073dd8
Compare
v2: updated to fix twister build issue in imx95/imx943 m/a core |
drivers/firmware/scmi/core.c
Outdated
ret = k_mutex_lock(&proto->rx->lock, K_USEC(SCMI_CHAN_LOCK_TIMEOUT_USEC)); | ||
if (ret < 0) { | ||
LOG_ERR("failed to acquire chan lock"); | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are returning with mutex locked. I think you might want to goto out_release_mutex
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if k_mutex_lock error, it not need to release mutex from my understand
drivers/firmware/scmi/mailbox.c
Outdated
|
||
#if defined(CONFIG_NXP_SCMI_BBM_HELPERS) | ||
uint32_t flags; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part should come in a different patch. Also, please abstract it under a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have removed this call back flow into another commit, thanks
This driver is NXP-specific. As such, it may only be used on NXP | ||
system that uses SCMI for cpu domain management operations. | ||
|
||
NXP - BBNSM management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in the commit message you should explain what BBNSM acronym stands for so that you can allow reviewers to understand what is this all about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated it in doc and commit description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see this scaling very well. So, how about moving all of the NXP vendor-specific stuff to a different page? Also, do we have a link to some sort of documentation fully describing the protocol? Kinda like what we have for Linux: https://elixir.bootlin.com/linux/v6.16.1/source/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
CC: @kartben would we bloat the documentation page if we were to add a full explanation of each vendor-specific protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, do you mean that we need add an directory scmi/vendor/nxp into zephyr/doc/hardware/arch? I can organize it if need, thanks
ping: @kartben , can you give some suggestion about this? thanks a lot
e073dd8
to
ea7ed13
Compare
V3: update pr according into @dbaluta comments
|
hi @dbaluta, have updated it according into your suggestion, other comments about it? thanks a lot |
|
Updated: removed not reasonable LOG info Thanks |
Sorry for the (very) long delay. I have this PR on my review TODO list. |
@LaurentiuM1234 ping |
Hi @LaurentiuM1234 , please help to review this pr again when you time free, thanks |
*/ | ||
typedef void (*scmi_channel_cb)(struct scmi_channel *chan); | ||
typedef void (*scmi_channel_cb)(struct scmi_channel *chan, struct scmi_message msg); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msg here doesnt look like a pointer. Or the comment is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions/comments below.
None of your commits should break the build if applied incrementally. If I try to build the hello_world
application with 6fbb947 and then 9a3dda1 applied, the build will break.
I believe @dbaluta has told you multiple times to break your changes into different patches, based on what the changes are trying to achieve. This, of course, applies to the whole PR, not just the areas on which @dbaluta commented.
As things stand now I don't believe this is ready for merge, which is why I'm blocking this.
drivers/firmware/scmi/nxp/bbm.c
Outdated
|
||
DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, nxp_scmi_bbm), NULL); | ||
|
||
extern uint32_t scmi_p2a_header_token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this driver added in the first commit? would it even compile? would it do anything useful without the notifications infrastructure?
drivers/firmware/scmi/nxp/bbm.c
Outdated
reply.len = sizeof(uint32_t); | ||
reply.content = &status; | ||
|
||
use_polling = k_is_pre_kernel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this variable - can just pass k_is_pre_kernel() to scmi_send_message()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated
drivers/firmware/scmi/nxp/bbm.c
Outdated
|
||
msg.hdr = SCMI_MESSAGE_HDR_MAKE(SCMI_PROTO_BBM_BBM_BUTTON_NOTIFY, SCMI_COMMAND, | ||
proto->id, 0); | ||
msg.len = sizeof(uint32_t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(flags)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, done
drivers/firmware/scmi/nxp/bbm.c
Outdated
msg.content = &flags; | ||
|
||
reply.hdr = msg.hdr; | ||
reply.len = sizeof(uint32_t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(status)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, done
drivers/firmware/scmi/nxp/bbm.c
Outdated
|
||
msg.hdr = SCMI_MESSAGE_HDR_MAKE(SCMI_PROTO_BBM_PROTOCOL_BUTTON_EVENT, SCMI_NOTIFICATION, | ||
proto->id, scmi_p2a_header_token++); | ||
msg.len = sizeof(uint32_t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(flags)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
#include <zephyr/drivers/firmware/scmi/shmem.h> | ||
#include <string.h> | ||
#include <zephyr/kernel.h> | ||
#include <zephyr/logging/log.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the point of incrementally introducing these changes to the BBM driver? this is a new protocol so might as well have everything introduced in a single commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put bbm driver as only single commit now, keep its readability of the code logic
drivers/firmware/scmi/core.c
Outdated
} | ||
|
||
static void scmi_core_reply_cb(struct scmi_channel *chan, struct scmi_message *msg) | ||
static void scmi_core_reply_cb(struct scmi_channel *chan, struct scmi_message msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this callback is meant for TX replies yet you're using it for notifications? I also don't understand the point of the second parameter and why it was randomly changed from a pointer to a struct scmi_message
to just struct scmi_message
in this commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, put tx/rx reply_cb into one scmi_core_reply_cb in core.c, but tx don't need another flow, just check its msg_type, its mainly deal with rx notification event, dispacher and send it into specific protocol
drivers/firmware/scmi/core.c
Outdated
#define SCMI_CHAN_LOCK_TIMEOUT_USEC 500 | ||
#define SCMI_CHAN_SEM_TIMEOUT_USEC 500 | ||
|
||
static sys_slist_t scmi_event_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a linked list? Can we not have each protocol keep an array of struct scmi_protocol_event
for all registered events?
The definition of struct scmi_protocol_event
would be:
struct scmi_protocol_event {
uint32_t id; /* event ID */
scmi_evt_callback_t cb; /* event callback */
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed list, added events_ptr into DT_SCMI_PROTOCOL_DEFINE_NODEV now, thanks
drivers/firmware/scmi/nxp/bbm.c
Outdated
|
||
static int scmi_nxp_bbm_event_init(void) | ||
{ | ||
return scmi_register_protocol_event_handler(&bbm_event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to also unregister protocol events? if not, then we can probably get around with binding the event array to the protocol structure at build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated it
dts/bindings/firmware/arm,scmi.yaml
Outdated
properties: | ||
shmem: | ||
type: phandle | ||
type: phandle-array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this not be phandles type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, its fine, corrected
SCMI P2A (Platform to Agent) communication is critical in platform-agent interactions. Previously, only A2P (Agent to Platform) was supported. This patch introduces the P2A workflow by adding support for `scmi_channel->rx` and `scmi_mbox_channel->rx`, treating the RX channel as an independent channel. For P2A RX channels, initialization is optional and depends on platform design. If both the protocol node and the base transport node do not specify an RX shmem region, the RX channel will not be initialized. Signed-off-by: Yongxu Wang <[email protected]>
Introduce a dedicated RX callback at the mailbox transport layer so TX and RX are dispatched via separate callbacks. Signed-off-by: Yongxu Wang <[email protected]>
Add scmi_shmem_clear_channel_status to clear channel status which diff a2p and p2a direction Signed-off-by: Yongxu Wang <[email protected]>
In the SCMI core layer, add msg_type validation before dispatch (e.g. RESP vs NOTIFY vs CMD). Messages with an unexpected or invalid type for the current context are logged Signed-off-by: Yongxu Wang <[email protected]>
Add support for protocol-level registration of notification events during static registration. Each protocol declares the notifications it needs at static init time. In the SCMI core layer, upon receiving a P2A notification, the core iterates over all registered protocols to locate the matching event and dispatches it to the appropriate protocol handler. Signed-off-by: Yongxu Wang <[email protected]>
Introduce scmi_read_message() to allow protocols to read the associated event payload when a P2A notification is received. This enables protocol drivers to process notification-specific data after the SCMI core dispatches the event. Signed-off-by: Yongxu Wang <[email protected]>
158935d
to
77defc5
Compare
This protocol is intended provide access to the battery-backed module, it contains persistent storage (GPR), an RTC, and the ON/OFF button. The protocol can also provide access to similar functions implemented via external board components. Added button P2A notification event for the BBM protocol. Signed-off-by: Yongxu Wang <[email protected]>
Extends the SCMI shared memory structure to phandles, allowing TX and RX channels to use different shared memory regions. This is necessary for platforms like i.MX95, where TX and RX notifications are mapped to different addresses. RX channel is not mandatory and depends on the platform's support Signed-off-by: Yongxu Wang <[email protected]>
Add scmi rx shmem and bbm node into imx95 m7 Signed-off-by: Yongxu Wang <[email protected]>
enable scmi bbm button function, m7 can get notification from scmi platform when press onoff button Signed-off-by: Yongxu Wang <[email protected]>
77defc5
to
c76d77e
Compare
Updated from LaurentiuM comments: |
|
test pass in imx95 m7 hello_word sample, The M7 core can continuously receive button messages from the SCMI platform.