IPC: IPM: Add ipm adsp driver & sanitycheck #22724
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| #!/usr/bin/env python3 | ||
| # | ||
| # Copyright (c) 2020 Intel Corporation | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from time import sleep | ||
| from mmap import mmap, ACCESS_COPY | ||
| from ctypes import c_uint8 | ||
|
|
||
| from lib.device import Device | ||
| import lib.registers as regs | ||
|
|
||
| MBOX = 0x91281000 | ||
| LENGTH = 0x1000 | ||
|
|
||
| with open("/dev/mem", "rb") as f: | ||
| mem_map = mmap(f.fileno(), LENGTH, access=ACCESS_COPY, offset=MBOX) | ||
| mem = (c_uint8 * LENGTH).from_buffer(mem_map) | ||
|
|
||
| def mailbox_poll_mem(dev): | ||
| while True: | ||
| if dev.dsp_hipct.value & regs.ADSP_IPC_HIPCT_BUSY: | ||
|
|
||
| # Use only id for passing character | ||
| line_len = dev.dsp_hipct.value & regs.ADSP_IPC_HIPCT_MSG | ||
|
|
||
| # Mask out internal bits | ||
| line_len &= 0x10FFFF | ||
|
|
||
| if line_len: | ||
| print(bytes(mem[:line_len]).decode()) | ||
|
|
||
| # Clear interrupt | ||
| dev.dsp_hipct.value |= regs.ADSP_IPC_HIPCT_BUSY | ||
| else: | ||
| sleep(0.005) | ||
|
|
||
| # Character passed in mailbox ID field | ||
| def mailbox_poll_char(dev): | ||
| while True: | ||
| if dev.dsp_hipct.value & regs.ADSP_IPC_HIPCT_BUSY: | ||
|
|
||
| # Use only id for passing character | ||
| character = dev.dsp_hipct.value & regs.ADSP_IPC_HIPCT_MSG | ||
|
|
||
| # Get readable character | ||
| character &= 0x10FFFF | ||
|
|
||
| print('{}'.format(chr(character)), end='') | ||
|
|
||
| # Clear interrupt | ||
| dev.dsp_hipct.value |= regs.ADSP_IPC_HIPCT_BUSY | ||
| else: | ||
| sleep(0.005) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| # Clear Done if needed | ||
| #dev.dsp_hipct.value |= regs.ADSP_IPC_HIPCT_BUSY | ||
|
|
||
| # Use Device library for controlling registers | ||
| device = Device() | ||
| device.open_device() | ||
|
|
||
| mailbox_poll_mem(device) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| /* | ||
| * Copyright (c) 2020 Intel Corporation | ||
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #include <kernel.h> | ||
| #include <device.h> | ||
| #include <init.h> | ||
| #include <drivers/ipm.h> | ||
|
|
||
| #include <logging/log.h> | ||
| LOG_MODULE_REGISTER(ipm_console, CONFIG_IPM_LOG_LEVEL); | ||
|
|
||
| #if defined(CONFIG_IPM_CONSOLE_IMMEDIATE) | ||
| struct device *ipm_dev; | ||
| #else | ||
|
|
||
| /* | ||
| * Stack for the message queue handling thread | ||
| */ | ||
| static K_THREAD_STACK_DEFINE(thread_stack, 1024); | ||
| static struct k_thread thread_data; | ||
|
|
||
| K_MSGQ_DEFINE(msg_queue, CONFIG_IPM_CONSOLE_LINE_BUF_LEN, | ||
| CONFIG_IPM_CONSOLE_QUEUE_SIZE, 4); | ||
|
|
||
| static void msgq_thread(void *arg1, void *arg2, void *arg3) | ||
| { | ||
| struct device *ipm_dev = arg1; | ||
|
|
||
| ARG_UNUSED(arg2); | ||
| ARG_UNUSED(arg3); | ||
|
|
||
| LOG_DBG("started thread"); | ||
|
|
||
| while (true) { | ||
| char buf[CONFIG_IPM_CONSOLE_LINE_BUF_LEN]; | ||
| int ret; | ||
|
|
||
| /* Get character from message queue */ | ||
| k_msgq_get(&msg_queue, buf, K_FOREVER); | ||
|
|
||
| /* Blocking write over IPM */ | ||
| ret = ipm_send(ipm_dev, 1, sizeof(buf), buf, sizeof(buf)); | ||
| if (ret) { | ||
| LOG_ERR("Error sending buf over IPM, ret %d", ret); | ||
| } | ||
|
|
||
| k_yield(); | ||
|
There was a problem hiding this comment. Why? This loop is going to jump straight into k_msgq_get() and block. Please remove this yield, or if it's really needed let's figure out the root cause of the hack (likely a thread priority messup) and fix that instead. |
||
| } | ||
| } | ||
| #endif /* ! CONFIG_IPM_CONSOLE_IMMEDIATE */ | ||
|
|
||
| static int console_out(int c) | ||
| { | ||
| static char buf[CONFIG_IPM_CONSOLE_LINE_BUF_LEN]; | ||
| static size_t len; | ||
|
There was a problem hiding this comment. Where is the locking for this data? It something happening up the stack somewhere that makes this safe? Remember that console output can happen from interrupt handlers and across SMP cores, and by default this code isn't safe. I think you need a spinlock around this data. If you don't need a spinlock, you need a comment explaining why. |
||
| int ret; | ||
|
|
||
| if (c != '\n' && len < sizeof(buf)) { | ||
| buf[len++] = c; | ||
| return c; | ||
| } | ||
|
|
||
| #if defined(CONFIG_IPM_CONSOLE_IMMEDIATE) | ||
| ret = ipm_send(ipm_dev, 1, len & ~BIT(31), buf, len); | ||
|
There was a problem hiding this comment. I'm seeing this pattern a lot in this pull request, where you're setting the high bit of the IPM message ID. Now, that's fine as far as IPM itself goes -- the ID is just an opaque value. But clearly it means something, and I can't see what. What's going on here? This needs documentation, I think. Right now the code you're adding is speaking an "ADSP IPM" variant protocol and won't work with default IPM backends. That's not crazy, since they're all device-specific, but it needs some kind of framework in software I think and not just a bunch of raw bits flying around. There was a problem hiding this comment. Here I have changed, following your suggestion, from sending one character to sending a line. This in not IPM but user-defined "console over IPM", kind of higher level protocol. There was a problem hiding this comment. OK, then maybe a "IPM_CONSOLE_MSGID(len)" macro would make this clearer? There was a problem hiding this comment. Oh, and note that this presumes a 32-bit-clean IPM message ID. Note that IPM implementations are allowed to have a smaller ID space, there's an API to query it. This should probably be matched with an __ASSERT (or BUILD_ASSERT) to make sure no one tries this with an incompatibile backend. There was a problem hiding this comment. OK, I've figured out what you're doing. You're clearing this bit because the hardware reserves the top bit of the message register for the BUSY flag and you don't want to clobber anything. But the API already gives you a way to inform users of that: |
||
| if (ret) { | ||
| LOG_ERR("Error sending character %c over IPM, ret %d", c, ret); | ||
| } | ||
| #else | ||
| ret = k_msgq_put(&msg_queue, buf, K_NO_WAIT); | ||
| if (ret) { | ||
| LOG_ERR("Error queueing buf, ret %d", ret); | ||
| } | ||
| #endif | ||
|
|
||
| memset(buf, 0, sizeof(buf)); | ||
| len = 0; | ||
|
|
||
| return c; | ||
| } | ||
|
|
||
| #if defined(CONFIG_STDOUT_CONSOLE) | ||
| extern void __stdout_hook_install(int (*hook)(int)); | ||
| #else | ||
| #define __stdout_hook_install(x) \ | ||
| do { /* nothing */ \ | ||
| } while ((0)) | ||
| #endif | ||
|
|
||
| #if defined(CONFIG_PRINTK) | ||
| extern void __printk_hook_install(int (*fn)(int)); | ||
| #else | ||
| #define __printk_hook_install(x) \ | ||
| do { /* nothing */ \ | ||
| } while ((0)) | ||
| #endif | ||
|
|
||
| /* Install printk/stdout hooks */ | ||
| static void ipm_console_hook_install(void) | ||
| { | ||
| __stdout_hook_install(console_out); | ||
| __printk_hook_install(console_out); | ||
| } | ||
|
|
||
| static int ipm_console_init(struct device *dev) | ||
| { | ||
| struct device *ipm; | ||
|
|
||
| ARG_UNUSED(dev); | ||
|
|
||
| LOG_DBG("IPM console initialization"); | ||
|
|
||
| ipm = device_get_binding(CONFIG_IPM_CONSOLE_ON_DEV_NAME); | ||
| if (!ipm) { | ||
| LOG_ERR("Cannot get %s", CONFIG_IPM_CONSOLE_ON_DEV_NAME); | ||
| return -ENODEV; | ||
| } | ||
|
|
||
| if (ipm_max_id_val_get(ipm) < 0xFF) { | ||
| LOG_ERR("IPM driver does not support 8 bit ID values"); | ||
| return -ENOTSUP; | ||
| } | ||
|
|
||
| #if defined(CONFIG_IPM_CONSOLE_IMMEDIATE) | ||
| ipm_dev = ipm; | ||
| #else | ||
| k_thread_create(&thread_data, thread_stack, | ||
| K_THREAD_STACK_SIZEOF(thread_stack), | ||
| msgq_thread, ipm, NULL, NULL, | ||
| CONFIG_MAIN_THREAD_PRIORITY - 1, | ||
|
There was a problem hiding this comment. If this driver is carrying console output (!), I can pretty much guarantee that "one higher than the app main thread priority" is the wrong value to hard code. I mean.... what happens when someone spins in a high priority thread and blocks console output? The system deadlocks silently, right? |
||
| 0, K_NO_WAIT); | ||
| k_thread_name_set(&thread_data, "ipm_console_thread"); | ||
| #endif /* CONFIG_IPM_CONSOLE_IMMEDIATE */ | ||
|
|
||
| ipm_console_hook_install(); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| /* Need to be initialized after IPM */ | ||
| SYS_INIT(ipm_console_init, POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT); | ||
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 had to look this up (because of course Python doesn't expose the system's mmap syscall with the same API).
"ACCESS_COPY" specifies a copy-on-write mapping. That makes basically zero sense to me for a shared region. Why on earth would this python script want its own copy of the memory that the other side was guaranteed not to see? That's not a "permission", and I don't understand why you'd want it here at all. What's going on?
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 have probably screwed something with this, should be
ACCESS_READ, will fix.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.
@andyross I cherry-picked this patch to #27804, addressed most of the comments except for this one, it needs some modification since from_buffer does not work with ACCESS_READ. I'll try to find some solution for this.