Skip to content
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

Add IPM console #27804

Merged
merged 10 commits into from
Sep 4, 2020
Merged

Add IPM console #27804

merged 10 commits into from
Sep 4, 2020

Conversation

finikorg
Copy link
Collaborator

@finikorg finikorg commented Aug 26, 2020

This PR adds IPM console which might be tested with up_squared_adsp.
Following options need to be enabled: CONSOLE & IPM and disable LOG_PRINTK

After that you can "flash" the board with:

$ west flash zephyr/boards/xtensa/up_squared_adsp/tools/up_squared_adsp_flash.sh <KEY>

And in another terminal

$ sudo boards/xtensa/up_squared_adsp/tools/mbterm.py 
*** Booting Zephyr OS build zephyr-v2.3.0-2086-g83a5af82007c  ***
Hello World! up_squared_adsp

Can be used with sanitycheck also, for example from inside zephyrproject:

$ zephyr/scripts/sanitycheck -M -c -p up_squared_adsp --device-test \
--west-flash="zephyr/boards/xtensa/up_squared_adsp/tools/up_squared_adsp_flash.sh,otc_private_key.pem" \
--device-serial-pty="$HOME/set_ambient zephyr/boards/xtensa/up_squared_adsp/tools/mbterm.py" \
-T zephyr/samples/hello_world \
-x="CONFIG_IPM=y" -x="CONFIG_CONSOLE=y" -x="CONFIG_LOG_PRINTK=n" -v

where $HOME/set_ambient is described here: #22724 (comment)

drivers/console/Kconfig Show resolved Hide resolved
drivers/console/ipm_console.c Show resolved Hide resolved
Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

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

I'm super confused.

Something wrong with the existing IPM console driver?
See drivers/console/ipm_console_sender.c, drivers/console/ipm_console_receiver.c

@finikorg
Copy link
Collaborator Author

finikorg commented Aug 27, 2020

I'm super confused.

Something wrong with the existing IPM console driver?
See drivers/console/ipm_console_sender.c, drivers/console/ipm_console_receiver.c

@andrewboie I can modify this ipm_console_sender.c to make it usable as a console. At the moment it is a kind of library used in tests/drivers/ipm/ fot tests only as names sender and receiver implies (It can be probably moved to tests)

So it was easier to write proper ipm_console then modify those console senders and receivers and also tests.

@andyross also does not like the method used to send character: See https://github.com/zephyrproject-rtos/zephyr/pull/22724/commits/2419dac1060bb11757519f17e43da7a694a27967 (github generates wrong link)

@andrewboie
Copy link
Contributor

At the moment it is a kind of library used in tests/drivers/ipm/ fot tests only as names sender and receiver implies (It can be probably moved to tests)

No, it's not a test library. We used it to forward console output on Arduino 101 from the ARC CPU to the Lakemont, it was enabled by the default for that board.

The name "sender" is the side sending the console messages (on Arduino 101, the ARC CPU)
The name "receiver" is the code running on the other CPU (on Arudiuno 101, the Lakemont) which receives the console data and prints it.

I don't care if you rewrite it but if you want to rewrite how IPM console works to make it line-buffered you need to remove all of this code or amend it. Not have two completely different implementations for the same thing in-tree, that's a hard NACK.

@finikorg
Copy link
Collaborator Author

finikorg commented Aug 27, 2020

At the moment it is a kind of library used in tests/drivers/ipm/ fot tests only as names sender and receiver implies (It can be probably moved to tests)

No, it's not a test library. We used it to forward console output on Arduino 101 from the ARC CPU to the Lakemont, it was enabled by the default for that board.

But now it is enabled only by test.

The name "sender" is the side sending the console messages (on Arduino 101, the ARC CPU)
The name "receiver" is the code running on the other CPU (on Arudiuno 101, the Lakemont) which receives the console data and prints it.

I don't care if you rewrite it but if you want to rewrite how IPM console works to make it line-buffered you need to remove all of this code or amend it. Not have two completely different implementations for the same thing in-tree, that's a hard NACK.

As I said @andyross gave strong NACK with one character per blocking send method, even wrote small essay ;).

So at the moment nobody uses this code, if everybody OK I will remove it. I am also OK reusing console sender, I still have support in my python terminal tool with getting console characters in ID field. I just need for that enabling console sender and removing or modifying test.

@finikorg finikorg force-pushed the ipm-console branch 2 times, most recently from efaa9e3 to 8129bf2 Compare August 31, 2020 11:06
@finikorg finikorg requested a review from galak as a code owner August 31, 2020 11:06
@github-actions github-actions bot added area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Aug 31, 2020
@github-actions github-actions bot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Aug 31, 2020
@finikorg finikorg requested a review from carlocaione as a code owner August 31, 2020 12:26
@github-actions github-actions bot added the area: ARM ARM (32-bit) Architecture label Aug 31, 2020
@carlescufi carlescufi requested a review from nordic-krch August 31, 2020 12:56
@@ -58,9 +58,6 @@ config CMSIS_V2_THREAD_MAX_STACK_SIZE
config CMSIS_V2_THREAD_DYNAMIC_STACK_SIZE
Copy link
Member

Choose a reason for hiding this comment

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

why is this being removed? I know it was used in A101, but there is nothing here that is related to 101 and we have a test. So I would leave it, we might get users for this in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nashif Do you mean drop the commit 58e0c009ab5fb0df07d33fa365f655d1d0956b6f ?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Add console over Inter Processor Mailboxes (IPM).
This is useful for AMP processors like ADSP found on up_squared board.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add @finikorg as codeowner for ipm_console.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add tool which can access ADSP console over IPM using Python device
library and polling Doorbell registers.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add defaults for IPM and IPM_CONSOLE when IPM and CONSOLE are
enabled.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Tweak ADSP initialization to catch early console output.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add default which takes the target device from DTS similar way to UART
console.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add IPM console DTS binding.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add IPM console to the board.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
By default, subprocess.Popen commands are supplied as a list of strings.
Using split() allows to use command with arguments, for example it is
possible to use following:

sanitycheck ... \
--device-serial-pty="set_ambient read_terminal.py" \
...

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Remove now unused configuration option.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Boards area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants