Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/clock_control/clock_control_arm_scmi.c
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,4 @@ static struct scmi_clock_data data;

DT_INST_SCMI_PROTOCOL_DEFINE(0, &scmi_clock_init, NULL, &data, NULL,
PRE_KERNEL_1, CONFIG_CLOCK_CONTROL_INIT_PRIORITY,
&scmi_clock_api);
&scmi_clock_api, NULL);
88 changes: 82 additions & 6 deletions drivers/firmware/scmi/core.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,55 @@ int scmi_status_to_errno(int scmi_status)
}
}

static void scmi_core_reply_cb(struct scmi_channel *chan)
static int scmi_core_handle_notification(int hdr)
{
uint32_t protocol_id, msg_id;
struct scmi_protocol_event *events;

protocol_id = SCMI_MESSAGE_HDR_EX_PROTOCOL(hdr);
msg_id = SCMI_MESSAGE_HDR_EX_MSGID(hdr);

STRUCT_SECTION_FOREACH(scmi_protocol, it) {
events = it->events;
if (!events || !events->cb) {
continue;
}

if (protocol_id == it->id) {
for (uint32_t num = 0; num < events->num_events; num++) {
if (msg_id == events->evts[num]) {
events->cb(msg_id);
return 0;
}
}
}
}

return -ENOENT;
}

static void scmi_core_reply_cb(struct scmi_channel *chan, int hdr)
{
int msg_type;
int status;

msg_type = SCMI_MESSAGE_HDR_EX_TYPE(hdr);

switch (msg_type) {
case SCMI_COMMAND:
break;
case SCMI_DELAYED_REPLY:
break;
case SCMI_NOTIFICATION:
status = scmi_core_handle_notification(hdr);
if (status) {
LOG_WRN("Scmi notification event not find");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/not find/not found. Also, do you want to print the status here?

break;
default:
LOG_WRN("Unexpected message type %u", msg_type);
}

if (!k_is_pre_kernel()) {
k_sem_give(&chan->sem);
}
Expand All @@ -63,11 +110,6 @@ static int scmi_core_setup_chan(const struct device *transport,
return 0;
}

/* no support for RX channels ATM */
if (!tx) {
return -ENOTSUP;
}

k_mutex_init(&chan->lock);
k_sem_init(&chan->sem, 0, 1);

Expand Down Expand Up @@ -216,6 +258,31 @@ int scmi_send_message(struct scmi_protocol *proto, struct scmi_message *msg,
}
}

int scmi_read_message(struct scmi_protocol *proto, struct scmi_message *msg)
{
int ret;

if (!proto->rx) {
return -ENODEV;
}

if (!proto->rx->ready) {
return -EINVAL;
}

/* read message from platform, such as notification event
*
* Unlike scmi_send_message, reading messages with scmi_read_message is not currently
* required in the PRE_KERNEL stage. The interrupt-based logic is used here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a

if (k_is_pre_kernel()) {
    return -EINVAL;
}

check here then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, added it now

*/
if (k_is_pre_kernel()) {
return -EINVAL;
}

ret = scmi_transport_read_message(proto->transport, proto->rx, msg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for ret here. Judt directly return scmi_transport_read_message.


return ret;
}
static int scmi_core_protocol_setup(const struct device *transport)
{
int ret;
Expand All @@ -226,6 +293,7 @@ static int scmi_core_protocol_setup(const struct device *transport)
#ifndef CONFIG_ARM_SCMI_TRANSPORT_HAS_STATIC_CHANNELS
/* no static channel allocation, attempt dynamic binding */
it->tx = scmi_transport_request_channel(transport, it->id, true);
it->rx = scmi_transport_request_channel(transport, it->id, false);
#endif /* CONFIG_ARM_SCMI_TRANSPORT_HAS_STATIC_CHANNELS */

if (!it->tx) {
Expand All @@ -236,6 +304,14 @@ static int scmi_core_protocol_setup(const struct device *transport)
if (ret < 0) {
return ret;
}

/* notification/delayed reply channel is optional */
if (it->rx) {
ret = scmi_core_setup_chan(transport, it->rx, false);
if (ret < 0) {
return ret;
}
}
}

return 0;
Expand Down
90 changes: 69 additions & 21 deletions drivers/firmware/scmi/mailbox.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,61 @@

#include <zephyr/logging/log.h>
#include "mailbox.h"
#include <zephyr/drivers/firmware/scmi/protocol.h>

LOG_MODULE_REGISTER(scmi_mbox);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/callbak/callback/ in the commit subject.


static void scmi_mbox_cb(const struct device *mbox,
mbox_channel_id_t channel_id,
void *user_data,
struct mbox_msg *data)
static int scmi_mbox_get_pending_msg(struct scmi_channel *chan,
struct scmi_message *msg)
{
uint32_t context;
int ret;
struct scmi_mbox_channel *mbox_chan = chan->data;

msg->hdr = 0x0;
msg->len = sizeof(uint32_t);
msg->content = &context;

ret = scmi_shmem_read_hdr(mbox_chan->shmem, msg);
if (ret < 0) {
LOG_ERR("failed to read message to shmem: %d", ret);
return ret;
}

return 0;
}

static void scmi_mbox_tx_reply_cb(const struct device *mbox,
mbox_channel_id_t channel_id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this into 2 patches:

  1. Preparation for introducing the RX channel. Here you rename scmi_mbox_cb -> scmi_mbox_tx_reply_cb and implement scmi_mbox_cb with the logic for supporting tx/rx direction but only TX implemented so far.
  2. Add the RX channel.

Much easier to review the transition like this.

void *user_data,
struct mbox_msg *data)
{
struct scmi_channel *scmi_chan = user_data;
struct scmi_message msg;

scmi_mbox_get_pending_msg(scmi_chan, &msg);

if (scmi_chan->cb) {
scmi_chan->cb(scmi_chan);
scmi_chan->cb(scmi_chan, msg.hdr);
}
}

static void scmi_mbox_notify_cb(const struct device *mbox,
mbox_channel_id_t channel_id,
void *user_data,
struct mbox_msg *data)
{
struct scmi_channel *scmi_chan = user_data;
struct scmi_mbox_channel *mbox_chan = scmi_chan->data;
const struct device *shmem = mbox_chan->shmem;
bool a2p = false;
struct scmi_message msg;

scmi_mbox_get_pending_msg(scmi_chan, &msg);

if (scmi_chan->cb) {
scmi_chan->cb(scmi_chan, msg.hdr);
scmi_shmem_clear_channel_status(shmem, a2p);
}
}

Expand Down Expand Up @@ -71,29 +114,34 @@ static int scmi_mbox_setup_chan(const struct device *transport,
{
int ret;
struct scmi_mbox_channel *mbox_chan;
struct mbox_dt_spec *tx_reply;
struct mbox_dt_spec *mbox_spec;

mbox_chan = chan->data;

if (!tx) {
return -ENOTSUP;
}

if (mbox_chan->tx_reply.dev) {
tx_reply = &mbox_chan->tx_reply;
if (tx) {
mbox_spec = mbox_chan->tx_reply.dev ? &mbox_chan->tx_reply : &mbox_chan->tx;
ret = mbox_register_callback_dt(mbox_spec, scmi_mbox_tx_reply_cb, chan);
if (ret < 0) {
LOG_ERR("failed to register reply cb on %s",
mbox_chan->tx_reply.dev ? "tx_reply" : "tx");
return ret;
}
} else {
tx_reply = &mbox_chan->tx;
}

ret = mbox_register_callback_dt(tx_reply, scmi_mbox_cb, chan);
if (ret < 0) {
LOG_ERR("failed to register tx reply cb");
return ret;
if (!mbox_chan->rx.dev) {
LOG_ERR("RX channel not defined");
return -ENOTSUP;
}
mbox_spec = &mbox_chan->rx;
ret = mbox_register_callback_dt(&mbox_chan->rx, scmi_mbox_notify_cb, chan);
if (ret < 0) {
LOG_ERR("failed to register notify cb on rx");
return ret;
}
}

ret = mbox_set_enabled_dt(tx_reply, true);
ret = mbox_set_enabled_dt(mbox_spec, true);
if (ret < 0) {
LOG_ERR("failed to enable tx reply dbell");
LOG_ERR("failed to enable %s dbell", tx ? "tx" : "rx");
}

/* enable interrupt-based communication */
Expand Down
59 changes: 40 additions & 19 deletions drivers/firmware/scmi/mailbox.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,34 @@
.tx_reply = _SCMI_MBOX_CHAN_DBELL(node_id, tx_reply), \
}

/* define private data for a protocol RX channel */
#define _SCMI_MBOX_CHAN_DEFINE_PRIV_RX(node_id, proto) \
static struct scmi_mbox_channel _SCMI_MBOX_CHAN_NAME(proto, 1) =\
{ \
.shmem = _SCMI_MBOX_SHMEM_BY_IDX(node_id, 1), \
.rx = _SCMI_MBOX_CHAN_DBELL(node_id, rx), \
}

/*
* Define a mailbox channel. This does two things:
* 1) Define the mandatory `struct scmi_channel` structure
* 2) Define the mailbox-specific private data for said
* channel (i.e: a struct scmi_mbox_channel)
*/
#define _SCMI_MBOX_CHAN_DEFINE(node_id, proto, idx) \
#define _SCMI_MBOX_CHAN_DEFINE_TX(node_id, proto, idx) \
_SCMI_MBOX_CHAN_DEFINE_PRIV_TX(node_id, proto); \
DT_SCMI_TRANSPORT_CHAN_DEFINE(node_id, idx, proto, \
&(_SCMI_MBOX_CHAN_NAME(proto, idx))); \

#define _SCMI_MBOX_CHAN_DEFINE_RX(node_id, proto, idx) \
_SCMI_MBOX_CHAN_DEFINE_PRIV_RX(node_id, proto); \
DT_SCMI_TRANSPORT_CHAN_DEFINE(node_id, idx, proto, \
&(_SCMI_MBOX_CHAN_NAME(proto, idx))); \

#define _SCMI_MBOX_CHAN_DEFINE_RX_OPTIONAL(node_id, proto, idx) \
COND_CODE_1(DT_PROP_HAS_IDX(node_id, shmem, idx), \
(_SCMI_MBOX_CHAN_DEFINE_RX(node_id, proto, idx)), \
())
/*
* Optionally define a mailbox channel for a protocol. This is optional
* because a protocol might not have a dedicated channel.
Expand All @@ -69,24 +86,26 @@
_SCMI_MBOX_CHAN_DEFINE_OPTIONAL(node_id, DT_REG_ADDR(node_id), 0)

/* define and validate base protocol TX channel */
#define DT_INST_SCMI_MBOX_BASE_CHAN_DEFINE(inst) \
BUILD_ASSERT(DT_INST_PROP_LEN(inst, mboxes) != 1 || \
(DT_INST_PROP_HAS_IDX(inst, shmem, 0) && \
DT_INST_PROP_HAS_NAME(inst, mboxes, tx)), \
"bad bidirectional channel description"); \
\
BUILD_ASSERT(DT_INST_PROP_LEN(inst, mboxes) != 2 || \
(DT_INST_PROP_HAS_NAME(inst, mboxes, tx) && \
DT_INST_PROP_HAS_NAME(inst, mboxes, tx_reply)), \
"bad unidirectional channel description"); \
\
BUILD_ASSERT(DT_INST_PROP_LEN(inst, shmem) == 1, \
"bad SHMEM count"); \
\
BUILD_ASSERT(DT_INST_PROP_LEN(inst, mboxes) <= 2, \
"bad mbox count"); \
\
_SCMI_MBOX_CHAN_DEFINE(DT_INST(inst, DT_DRV_COMPAT), SCMI_PROTOCOL_BASE, 0)
#define DT_INST_SCMI_MBOX_BASE_CHAN_DEFINE(inst) \
BUILD_ASSERT(DT_INST_PROP_LEN(inst, mboxes) != 1 || \
(DT_INST_PROP_HAS_IDX(inst, shmem, 0) && \
DT_INST_PROP_HAS_NAME(inst, mboxes, tx)), \
"bad bidirectional channel description"); \
\
BUILD_ASSERT(DT_INST_PROP_LEN(inst, mboxes) != 2 || \
((DT_INST_PROP_HAS_NAME(inst, mboxes, tx) && \
(DT_INST_PROP_HAS_NAME(inst, mboxes, tx_reply) || \
DT_INST_PROP_HAS_NAME(inst, mboxes, rx)))), \
"bad unidirectional channel description"); \
\
BUILD_ASSERT(DT_INST_PROP_LEN(inst, shmem) <= 2, \
"bad SHMEM count"); \
\
BUILD_ASSERT(DT_INST_PROP_LEN(inst, mboxes) <= 2, \
"bad mbox count"); \
\
_SCMI_MBOX_CHAN_DEFINE_TX(DT_INST(inst, DT_DRV_COMPAT), SCMI_PROTOCOL_BASE, 0) \
_SCMI_MBOX_CHAN_DEFINE_RX_OPTIONAL(DT_INST(inst, DT_DRV_COMPAT), SCMI_PROTOCOL_BASE, 1) \

/*
* Define the mailbox-based transport layer. What this does is:
Expand All @@ -112,6 +131,8 @@ struct scmi_mbox_channel {
struct mbox_dt_spec tx;
/* TX reply dbell */
struct mbox_dt_spec tx_reply;
/* RX dbell */
struct mbox_dt_spec rx;
};

#endif /* _ZEPHYR_DRIVERS_FIRMWARE_SCMI_MAILBOX_H_ */
2 changes: 1 addition & 1 deletion drivers/firmware/scmi/nxp/cpu.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include <zephyr/drivers/firmware/scmi/nxp/cpu.h>
#include <zephyr/kernel.h>

DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, nxp_scmi_cpu), NULL);
DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, nxp_scmi_cpu), NULL, NULL);

int scmi_cpu_sleep_mode_set(struct scmi_cpu_sleep_mode_config *cfg)
{
Expand Down
2 changes: 1 addition & 1 deletion drivers/firmware/scmi/pinctrl.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include <zephyr/drivers/firmware/scmi/pinctrl.h>
#include <zephyr/kernel.h>

DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, arm_scmi_pinctrl), NULL);
DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, arm_scmi_pinctrl), NULL, NULL);

int scmi_pinctrl_settings_configure(struct scmi_pinctrl_settings *settings)
{
Expand Down
2 changes: 1 addition & 1 deletion drivers/firmware/scmi/power.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include <string.h>
#include <zephyr/kernel.h>

DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, arm_scmi_power), NULL);
DT_SCMI_PROTOCOL_DEFINE_NODEV(DT_INST(0, arm_scmi_power), NULL, NULL);

struct scmi_power_state_get_reply {
int32_t status;
Expand Down
Loading