Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
253 changes: 136 additions & 117 deletions drivers/mbox/mbox_ti_secproxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,18 @@ LOG_MODULE_REGISTER(ti_secure_proxy);
#define RT_THREAD_STATUS_CUR_CNT_MASK GENMASK(7, 0)

#define SCFG_THREAD_CTRL 0x1000
#define SCFG_THREAD_CTRL_DIR_SHIFT 31
#define SCFG_THREAD_CTRL_DIR_MASK BIT(31)

#define SEC_PROXY_THREAD(base, x) ((base) + (0x1000 * (x)))
#define THREAD_IS_RX 1
#define THREAD_IS_TX 0

#define SECPROXY_MAILBOX_NUM_MSGS 5
#define MAILBOX_MAX_CHANNELS 32
#define MAILBOX_MBOX_SIZE 60
#define MAILBOX_MAX_CHANNELS 32
#define MAILBOX_MBOX_SIZE 60

#define SEC_PROXY_DATA_START_OFFS 0x4
#define SEC_PROXY_DATA_END_OFFS 0x3c

#define SEC_PROXY_TIMEOUT_US 1000000
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove the busy wait? I'm not saying I'm opposed to this change, but you need to explain it.

And also it would be good to split that change into its own patch. You really should try to do that for each of the bullet points in your commit messages, each commit should do one single logical thing, having a list of different changes in a commit message is a sign that you are doing too much in one commit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, the rationale behind this change was that the timer is not initialized until PRE_KERNEL_2, hence it can cause issues to use busy_wait here. Alternatively, we can also add a check to see whether we are pre_kernel_* stages or not, but we concluded that this busy_wait is not necessary either way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The commits have been split up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

but we concluded that this busy_wait is not necessary either way.

How was that concluded? Looking at other implementations of this TI-SCI driver like the one in OP-TEE and TF-A, I see timeout waits.

You also do not explain it in the new split up commit either, the commit message just has:

Update secproxy_verify_thread to remove busy wait.

I can read as much in the code itself, commit messages should tell the "why" something is being changed. I think I've given this feedback before to you, please start working on your commit messages.

Copy link
Copy Markdown
Contributor

@natto1784 natto1784 Jan 28, 2026

Choose a reason for hiding this comment

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

The idea was that since every core has its own dedicated channel for communication on the mailbox instance, a simple check to see whether its empty (for RX) or full (for TX) should work in theory.

However, I do understand now that this is not ideal and retract my conclusion. The busy wait can be kept here, but should go inside a !k_is_pre_kernel() check, because we cannot really rely on timers such as dmtimer since it is not necessarily initialized at this point. This is due to how k_busy_wait is implemented - speaking of which, maybe we can do nops to busy wait if dmtimer is not initialized.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also U-boot, Linux and MCU+SDK dont have a busy wait

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not interested in what other implementations do, some have the wait, some do not, what I care about is what is correct. We had the polling timeout wait before, but you are removing it without an explanation beyond you finding some other implementations without this feature.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated commit message to include rationale

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see any update to the commit message with any rationale. Seems you have just repeated that found some implementations of the same without the wait. I can find implementations with the wait (OP-TEE/TF-A) so that does tell us which way is correct and why.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

b8e33f1 and the other commit messages have been updated for clarity.


#define GET_MSG_SEQ(buffer) ((uint32_t *)buffer)[1]
struct secproxy_thread {
mem_addr_t target_data;
Expand Down Expand Up @@ -83,20 +79,21 @@ struct secproxy_mailbox_config {
DEVICE_MMIO_NAMED_ROM(target_data);
DEVICE_MMIO_NAMED_ROM(rt);
DEVICE_MMIO_NAMED_ROM(scfg);
uint32_t irq;
int32_t interrupts[MAILBOX_MAX_CHANNELS];
};

static inline int secproxy_verify_thread(struct secproxy_thread *spt, uint8_t dir)
{
/* Check for any errors already available */
if (sys_read32(spt->rt + RT_THREAD_STATUS) & RT_THREAD_STATUS_ERROR_MASK) {
uint32_t status = sys_read32(spt->rt + RT_THREAD_STATUS);

if (status & RT_THREAD_STATUS_ERROR_MASK) {
LOG_ERR("Thread is corrupted, cannot send data.\n");
return -EINVAL;
}

/* Make sure thread is configured for right direction */
if ((sys_read32(spt->scfg + SCFG_THREAD_CTRL) & SCFG_THREAD_CTRL_DIR_MASK) !=
(dir << SCFG_THREAD_CTRL_DIR_SHIFT)) {
if (FIELD_GET(SCFG_THREAD_CTRL_DIR_MASK, sys_read32(spt->scfg + SCFG_THREAD_CTRL)) != dir) {
if (dir == THREAD_IS_TX) {
LOG_ERR("Trying to send data on RX Thread\n");
} else {
Expand All @@ -105,105 +102,85 @@ static inline int secproxy_verify_thread(struct secproxy_thread *spt, uint8_t di
return -EINVAL;
}

/* Check the message queue before sending/receiving data */
int timeout_ms = SEC_PROXY_TIMEOUT_US;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

int waited_ms = 0;
const int poll_interval_ms = 1000;

while (!(sys_read32(spt->rt + RT_THREAD_STATUS) & RT_THREAD_STATUS_CUR_CNT_MASK)) {
k_busy_wait(poll_interval_ms);
waited_ms += poll_interval_ms;
if (waited_ms >= timeout_ms) {
LOG_ERR("Timeout waiting for thread to %s\n",
(dir == THREAD_IS_TX) ? "empty" : "fill");
return -ETIMEDOUT;
if ((status & RT_THREAD_STATUS_CUR_CNT_MASK) == 0) {
if (dir == THREAD_IS_TX) {
return -EBUSY;
} else {
return -ENODATA;
}
}

return 0;
}

static void secproxy_mailbox_isr(const struct device *dev)
static void secproxy_mailbox_isr(const struct device *dev, uint32_t channel)
{
struct secproxy_mailbox_data *data = DEV_DATA(dev);
struct secproxy_thread spt;
uint32_t data_word;
mem_addr_t data_reg;

for (int i_channel = 0; i_channel < MAILBOX_MAX_CHANNELS; i_channel++) {
if (!data->channel_enable[i_channel]) {
continue;
}

spt.target_data = SEC_PROXY_THREAD(DEV_TDATA(dev), i_channel);
spt.rt = SEC_PROXY_THREAD(DEV_RT(dev), i_channel);
spt.scfg = SEC_PROXY_THREAD(DEV_SCFG(dev), i_channel);

uint32_t status = sys_read32(spt.rt + RT_THREAD_STATUS);

if (status & RT_THREAD_STATUS_ERROR_MASK) {
LOG_ERR("Thread %d error state detected in ISR", i_channel);
continue;
}

if (secproxy_verify_thread(&spt, THREAD_IS_RX)) {
LOG_ERR("Thread %d is in error state\n", i_channel);
continue;
}
if (!data->channel_enable[channel]) {
return;
}

if (!(sys_read32(spt.rt) & 0x7F)) {
continue;
}
spt.target_data = SEC_PROXY_THREAD(DEV_TDATA(dev), channel);
spt.rt = SEC_PROXY_THREAD(DEV_RT(dev), channel);
spt.scfg = SEC_PROXY_THREAD(DEV_SCFG(dev), channel);

data_reg = spt.target_data + SEC_PROXY_DATA_START_OFFS;
size_t msg_len = MAILBOX_MBOX_SIZE;
size_t num_words = msg_len / sizeof(uint32_t);
size_t i;
struct rx_msg *rx_data = data->user_data[i_channel];
if (secproxy_verify_thread(&spt, THREAD_IS_RX)) {
/* Silent failure */
return;
}

if (!rx_data || !rx_data->buf) {
LOG_ERR("No buffer provided for channel %d", i_channel);
continue;
}
data_reg = spt.target_data + SEC_PROXY_DATA_START_OFFS;
size_t msg_len = MAILBOX_MBOX_SIZE;
size_t num_words = msg_len / sizeof(uint32_t);
size_t i;
struct rx_msg *rx_data = data->user_data[channel];

if (!rx_data || !rx_data->buf) {
LOG_ERR("No buffer provided for channel %d", channel);
return;
}

if (rx_data->size < MAILBOX_MBOX_SIZE) {
LOG_ERR("Buffer too small for channel %d", i_channel);
continue;
}
if (rx_data->size < MAILBOX_MBOX_SIZE) {
LOG_ERR("Buffer too small for channel %d", channel);
return;
}

uint8_t *buf = (uint8_t *)rx_data->buf;
uint8_t *buf = (uint8_t *)rx_data->buf;

/* Copy full words */
for (i = 0; i < num_words; i++) {
data_word = sys_read32(data_reg);
memcpy(&buf[i * 4], &data_word, sizeof(uint32_t));
data_reg += sizeof(uint32_t);
}
/* Copy full words */
for (i = 0; i < num_words; i++) {
data_word = sys_read32(data_reg);
memcpy(&buf[i * 4], &data_word, sizeof(uint32_t));
data_reg += sizeof(uint32_t);
}

/* Handle trail bytes */
size_t trail_bytes = msg_len % sizeof(uint32_t);
/* Handle trail bytes */
size_t trail_bytes = msg_len % sizeof(uint32_t);

if (trail_bytes) {
uint32_t data_trail = sys_read32(data_reg);
if (trail_bytes) {
uint32_t data_trail = sys_read32(data_reg);

i = msg_len - trail_bytes;
i = msg_len - trail_bytes;

while (trail_bytes--) {
buf[i++] = data_trail & 0xff;
data_trail >>= 8;
}
while (trail_bytes--) {
buf[i++] = data_trail & 0xff;
data_trail >>= 8;
}
}

/* Ensure we read the last register if we haven't already */
if (data_reg <= (spt.target_data + SEC_PROXY_DATA_END_OFFS)) {
sys_read32(spt.target_data + SEC_PROXY_DATA_END_OFFS);
}
/* Ensure we read the last register if we haven't already */
if (data_reg <= (spt.target_data + SEC_PROXY_DATA_END_OFFS)) {
sys_read32(spt.target_data + SEC_PROXY_DATA_END_OFFS);
}

rx_data->size = msg_len;
rx_data->seq = GET_MSG_SEQ(buf);
if (data->cb[i_channel]) {
data->cb[i_channel](dev, i_channel, data->user_data[i_channel], NULL);
}
rx_data->size = msg_len;
rx_data->seq = GET_MSG_SEQ(buf);
if (data->cb[channel]) {
data->cb[channel](dev, channel, data->user_data[channel], NULL);
}
}

Expand All @@ -214,6 +191,7 @@ static int secproxy_mailbox_send(const struct device *dev, uint32_t channel,
struct secproxy_mailbox_data *data = DEV_DATA(dev);
mem_addr_t data_reg;
k_spinlock_key_t key;
uint32_t status;

if (msg == NULL || msg->data == NULL) {
LOG_ERR("Invalid parameters");
Expand All @@ -235,17 +213,10 @@ static int secproxy_mailbox_send(const struct device *dev, uint32_t channel,
spt.rt = SEC_PROXY_THREAD(DEV_RT(dev), channel);
spt.scfg = SEC_PROXY_THREAD(DEV_SCFG(dev), channel);

if (secproxy_verify_thread(&spt, THREAD_IS_TX)) {
LOG_ERR("Thread is in error state\n");
k_spin_unlock(&data->lock, key);
return -EBUSY;
}

uint32_t status = sys_read32(spt.rt + RT_THREAD_STATUS);

if ((status & RT_THREAD_STATUS_CUR_CNT_MASK) == (SECPROXY_MAILBOX_NUM_MSGS)) {
status = secproxy_verify_thread(&spt, THREAD_IS_TX);
if (status != 0) {
k_spin_unlock(&data->lock, key);
return -EBUSY;
return status;
}

if (msg->size > MAILBOX_MBOX_SIZE) {
Expand Down Expand Up @@ -324,6 +295,21 @@ static uint32_t secproxy_mailbox_max_channels_get(const struct device *dev)
return MAILBOX_MAX_CHANNELS;
}

static void secproxy_mailbox_flush_thread(const struct device *dev, uint32_t channel)
{
struct secproxy_thread spt;

spt.target_data = SEC_PROXY_THREAD(DEV_TDATA(dev), channel);
spt.rt = SEC_PROXY_THREAD(DEV_RT(dev), channel);
spt.scfg = SEC_PROXY_THREAD(DEV_SCFG(dev), channel);

/* Drain all pending messages from the thread */
while ((sys_read32(spt.rt + RT_THREAD_STATUS) & RT_THREAD_STATUS_CUR_CNT_MASK) > 0) {
/* Read from the last data register to consume one message */
(void)sys_read32(spt.target_data + SEC_PROXY_DATA_END_OFFS);
}
}

static int secproxy_mailbox_set_enabled(const struct device *dev, uint32_t channel, bool enable)
{
const struct secproxy_mailbox_config *cfg = DEV_CFG(dev);
Expand All @@ -338,13 +324,19 @@ static int secproxy_mailbox_set_enabled(const struct device *dev, uint32_t chann
return -EALREADY;
}

if (cfg->interrupts[channel] < 0) {
Comment thread
Kronosblaster marked this conversation as resolved.
LOG_ERR("No interrupt configured for channel %d", channel);
return -EINVAL;
}

key = k_spin_lock(&data->lock);
data->channel_enable[channel] = enable;

if (enable) {
irq_enable(cfg->irq);
secproxy_mailbox_flush_thread(dev, channel);
irq_enable(cfg->interrupts[channel]);
} else {
irq_disable(cfg->irq);
irq_disable(cfg->interrupts[channel]);
}

k_spin_unlock(&data->lock, key);
Expand All @@ -360,27 +352,54 @@ static DEVICE_API(mbox, secproxy_mailbox_driver_api) = {
.set_enabled = secproxy_mailbox_set_enabled,
};

#define MAILBOX_INSTANCE_DEFINE(idx) \
static struct secproxy_mailbox_data secproxy_mailbox_##idx##_data; \
const static struct secproxy_mailbox_config secproxy_mailbox_##idx##_config = { \
DEVICE_MMIO_NAMED_ROM_INIT_BY_NAME(target_data, DT_DRV_INST(idx)), \
DEVICE_MMIO_NAMED_ROM_INIT_BY_NAME(rt, DT_DRV_INST(idx)), \
DEVICE_MMIO_NAMED_ROM_INIT_BY_NAME(scfg, DT_DRV_INST(idx)), \
.irq = DT_INST_IRQN(idx), \
}; \
static int secproxy_mailbox_##idx##_init(const struct device *dev) \
{ \
DEVICE_MMIO_NAMED_MAP(dev, target_data, K_MEM_CACHE_NONE); \
DEVICE_MMIO_NAMED_MAP(dev, rt, K_MEM_CACHE_NONE); \
DEVICE_MMIO_NAMED_MAP(dev, scfg, K_MEM_CACHE_NONE); \
IRQ_CONNECT(DT_INST_IRQN(idx), DT_INST_IRQ(idx, priority), secproxy_mailbox_isr, \
DEVICE_DT_INST_GET(idx), \
COND_CODE_1(DT_INST_IRQ_HAS_CELL(idx, flags), \
(DT_INST_IRQ(idx, flags)), (0))); \
return 0; \
} \
DEVICE_DT_INST_DEFINE(idx, secproxy_mailbox_##idx##_init, NULL, \
&secproxy_mailbox_##idx##_data, &secproxy_mailbox_##idx##_config, \
#define SECPROXY_THREAD_ISR(i, idx) \
COND_CODE_1(DT_INST_IRQ_HAS_NAME(idx, DT_CAT(rx_, i)), \
( \
static void secproxy_mailbox_isr_##idx##_##i(const struct device *dev) \
{ \
secproxy_mailbox_isr(dev, i); \
} \
), \
())

/* Generate IRQ number or -1 for array initialization */
#define SECPROXY_IRQ_OR_INVALID(i, inst) \
COND_CODE_1(DT_INST_IRQ_HAS_NAME(inst, DT_CAT(rx_, i)), \
(DT_INST_IRQ_BY_NAME(inst, DT_CAT(rx_, i), irq)), \
(-1))

#define SECPROXY_IRQ_CONNECT(i, inst) \
COND_CODE_1(DT_INST_IRQ_HAS_NAME(inst, DT_CAT(rx_, i)), \
( \
IRQ_CONNECT(DT_INST_IRQ_BY_NAME(inst, DT_CAT(rx_, i), irq), \
DT_INST_IRQ_BY_NAME(inst, DT_CAT(rx_, i), priority), \
secproxy_mailbox_isr_##inst##_##i, \
DEVICE_DT_INST_GET(inst), \
COND_CODE_1(DT_INST_IRQ_HAS_CELL(inst, flags), \
(DT_INST_IRQ_BY_NAME(inst, DT_CAT(rx_, i), flags)), \
(0))); \
), \
())

#define MAILBOX_INSTANCE_DEFINE(idx) \
LISTIFY(MAILBOX_MAX_CHANNELS, SECPROXY_THREAD_ISR, (), idx) \
static struct secproxy_mailbox_data secproxy_mailbox_##idx##_data; \
const static struct secproxy_mailbox_config secproxy_mailbox_##idx##_config = { \
DEVICE_MMIO_NAMED_ROM_INIT_BY_NAME(target_data, DT_DRV_INST(idx)), \
DEVICE_MMIO_NAMED_ROM_INIT_BY_NAME(rt, DT_DRV_INST(idx)), \
DEVICE_MMIO_NAMED_ROM_INIT_BY_NAME(scfg, DT_DRV_INST(idx)), \
.interrupts = {LISTIFY(MAILBOX_MAX_CHANNELS, \
SECPROXY_IRQ_OR_INVALID, (,), idx) }}; \
static int secproxy_mailbox_##idx##_init(const struct device *dev) \
{ \
DEVICE_MMIO_NAMED_MAP(dev, target_data, K_MEM_CACHE_NONE); \
DEVICE_MMIO_NAMED_MAP(dev, rt, K_MEM_CACHE_NONE); \
DEVICE_MMIO_NAMED_MAP(dev, scfg, K_MEM_CACHE_NONE); \
LISTIFY(MAILBOX_MAX_CHANNELS, SECPROXY_IRQ_CONNECT, (;), idx) \
return 0; \
} \
DEVICE_DT_INST_DEFINE(idx, secproxy_mailbox_##idx##_init, NULL, \
&secproxy_mailbox_##idx##_data, &secproxy_mailbox_##idx##_config, \
PRE_KERNEL_1, CONFIG_MBOX_INIT_PRIORITY, \
&secproxy_mailbox_driver_api)

Expand Down
10 changes: 0 additions & 10 deletions dts/arm/ti/am64x_r5.dtsi
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,6 @@
};
};

&dmsc {
ti,host-id = <36>;
mboxes = <&secure_proxy_main 2>, <&secure_proxy_main 3>;
};

&secure_proxy_main {
interrupts = <0 65 IRQ_TYPE_EDGE IRQ_DEFAULT_PRIORITY>;
interrupt-parent = <&vim>;
};

&main_timer0 {
interrupts = <0 152 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>;
interrupt-parent = <&vim>;
Expand Down
12 changes: 12 additions & 0 deletions dts/arm/ti/am64x_r5f0_0.dtsi
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,15 @@
interrupts = <0 0 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>;
interrupt-parent = <&vim>;
};

&dmsc {
ti,host-id = <36>;
mboxes = <&secure_proxy_main 2>, <&secure_proxy_main 3>;
};

&secure_proxy_main {
interrupt-names = "rx_0", "rx_2";
interrupts = <0 64 IRQ_TYPE_EDGE IRQ_DEFAULT_PRIORITY>,
<0 65 IRQ_TYPE_EDGE IRQ_DEFAULT_PRIORITY>;
interrupt-parent = <&vim>;
};
Loading
Loading