IPC: IPM: Add ipm adsp driver & sanitycheck #22724

finikorg · 2020-02-11T14:23:28Z

Add IPM ADSP driver, DT configuration, ipm_echo sample, IPM console and documentation.
Added sanitycheck support for device-testing of up_squared_adsp.

Testing:
Build sample with

$ west build -p -b up_squared_adsp zephyr/samples/hello_world/

Flash with command

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

Open simplest polling mailbox terminal with

$ sudo boards/xtensa/up_squared_adsp/tools/mbterm.py 
*** Booting Zephyr OS build zephyr-v2.2.0-1620-g4484b8eff384  ***
Hello World! up_squared_adsp

Try sanitycheck as superuser with

# 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 zephyr/boards/xtensa/up_squared_adsp/tools/mbterm.py

Running as regular user with ambient (more about ambient: #22724 (comment))

$ ~/work/ambient/set_ambient 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 zephyr/boards/xtensa/up_squared_adsp/tools/mbterm.py -v

zephyrbot · 2020-02-11T14:35:19Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

dcpleung · 2020-02-11T16:51:19Z

Could you squash the two sample commits together?

drivers/ipm/ipm_adsp.c

andyross

Some notes on first reading. Is there a host-side driver integration somewhere?

andyross · 2020-02-13T17:11:31Z

drivers/ipm/ipm_adsp.c

+	/* IPC Target Busy Interrupt Enable (IPCTBIE) */
+	if (dipct & IPC_DIPCT_BUSY && dipcctl & IPC_DIPCCTL_IPCTBIE) {
+		/* Mask Busy interrupt */
+		ipc_write(IPC_DIPCCTL, dipcctl & ~IPC_DIPCCTL_IPCTBIE);


So... you're in the ISR here already. Why is this masking the interrupt it's servicing? I'm guessing that this is either needless, or what is happening here is you're masking delivery of the interrupt to the OTHER side of the IPM channel while the shared state is indeterminate, in which case you really need a comment here (and probably need some other locking I'm guessing?). Or maybe the mask/unmask sequence is needed to properly ACK the interrupt? Same thing, needs a comment.

I do not have good documentation for this, only register description. I have followed what SOF is doing for IPC IRQ handler:
https://github.com/thesofproject/sof/blob/3dd843a46defd5234a517991a7c0049c66233b1d/src/drivers/intel/cavs/ipc.c#L64-L137
I will add more comments.

If I cast my mind back, we cant clear this IRQ until we have finished processing the IPC (which may take some time) hence the mask. Clearing could also interrupt the host on some HW too. So short term this flow is fine as it survives 25k flood IPC pings. Long term we should be operating a msg queue where we can clear the IRQ and process the IPC offline (and allow other IPC to be sent).

andyross · 2020-02-13T17:13:00Z

drivers/ipm/ipm_adsp.c

+	if (dipcie & IPC_DIPCIE_DONE && dipcctl & IPC_DIPCCTL_IPCIDIE) {
+		/* Mask Done interrupt */
+		ipc_write(IPC_DIPCCTL,
+			  ipc_read(IPC_DIPCCTL) & ~IPC_DIPCCTL_IPCIDIE);


Same comment as above. Need clarity on what is being masked and why.

andyross · 2020-02-13T17:25:50Z

drivers/ipm/ipm_adsp.c

+	}
+
+	memcpy((void *)IPM_ADSP_MAILBOX_OUT, data, size);
+	SOC_DCACHE_FLUSH((void *)IPM_ADSP_MAILBOX_OUT, size);


Given that this is an app-visible API, you almost certainly want to check size vs. MAILBOX_OUT_SIZE to avoid scribbling over shared areas and potentially confusing/crashing the host kernel. Or at the very least check it in an __ASSERT().

So this supports only one message at a time? A quick grep says that the outbox region is 4k, I think, so it seems like some queuing ability would really improve things.

About queuing. API mentions:

zephyr/include/drivers/ipm.h

Lines 116 to 118 in bd5048c

* finishes. If there is deferred processing on the remote side,

* or you would like to queue outgoing messages and wait on an

* event/semaphore, you can implement that in a high-level driver

Or at the very least check it in an __ASSERT().

Added BUILD_ASSERT.

andyross · 2020-02-13T17:28:20Z

drivers/ipm/ipm_adsp.c

+			/* Use zero copy */
+			data->callback(data->callback_ctx,
+				       dipct & IPC_DIPCI_MSG_MASK,
+				       (void *)IPM_ADSP_MAILBOX_IN);


Similar comment to the send side: this supports only one message in the buffer at a time. But this buffer is 8k, and IPM is designed to be asynchronous, so some queing ability would definitely be possible and a good idea.

andyross · 2020-02-13T18:09:55Z

drivers/console/ipm_console.c

+static int console_out(int c)
+{
+	/* Send character with busy wait */
+	ipm_send(ipm_console_device, 1, c & ~BIT(31), NULL, 0);


OK, excellent example of why you want queuing in this driver. This is going to stall waiting on the other side to take an interrupt and clear the busy flat for... every... byte... of... log... output... Some linux driver developer is going to be coming at you with a shiv made from the shards of a smartphone screen.

FWIW: accumulating a local line buffer and flushing when it gets full or on newline would be a very reasonable workaround too.

I took it initially from drivers/console/ipm_console_sender.c

zephyr/drivers/console/ipm_console_sender.c

Line 28 in 49bf8b1

ipm_send(ipm_console_device, 1, character, NULL, 0);

I think we can just send without busy wait. If we get BUSY we can just drop the character.

For the testing it was easier to set this flag to verify that it works.

I have now immediate log with line buffer and another option using message queue msgq.

finikorg · 2020-02-14T07:53:04Z

Some notes on first reading. Is there a host-side driver integration somewhere?

For the host side I use devmem2, in this patch set there is README
https://github.com/finikorg/zephyr/tree/ipm/samples/subsys/ipc/ipm_echo

lgirdwood

I think in general we need to look at de-duplicating code in APL and S1000 ADSP to create a SoC family for all CAVS IP versions.

lgirdwood · 2020-02-15T15:51:45Z

drivers/ipm/ipm_adsp.c

+}
+
+static int ipm_adsp_send(struct device *dev, int wait, u32_t id,
+			 const void *data, int size)


We should really have the IPC IP programming flows under the SoC family since this function will be used by all CAVS v1.5 DSPs and not just APL..

At the moment I have access only to up_squared board, so it is the only one supported platform. I can test it also on other platforms when they become available.
BTW: How the code would be different for other SOCs?

Best to have a look at the IPC drivers in SOF, but generally the IPC doorbell HW has changed between 1st gen (BYT, CHT), 2nd gen (HSW, BDW), 3rd ((SKL - APL), 4th (CNL+). I've started some work on this now, will send a patch for you to review hopefully tomorrow.

I see the difference with CAVS_VERSION_1_5 and others in https://github.com/thesofproject/sof/blob/master/src/drivers/intel/cavs/ipc.c
I can add support for other boards but I have only up_squared_adsp.

lgirdwood · 2020-02-15T16:29:20Z

soc/xtensa/intel_apl_adsp/adsp.c

@@ -78,6 +78,14 @@ static void prepare_host_windows(void)
 	SOC_DCACHE_FLUSH((void *)(HP_SRAM_WIN0_BASE + SRAM_REG_FW_END),
 			 HP_SRAM_WIN0_SIZE - SRAM_REG_FW_END);

+	if (IS_ENABLED(CONFIG_IPM_ADSP)) {


I'm thinking long term we should build the extended FW boot data (i.e. data we send to host about window mappings and some FW capabilities via DT). Any thoughts on this, not something we need immediately but worth considering ?

At the moment we send window mappings:

zephyr/soc/xtensa/intel_apl_adsp/adsp.c

Lines 91 to 107 in 93b78b7

/*

* Sends the firmware ready message so the firmware loader can

* map the host windows.

*/

static void send_fw_ready(void)

{

memcpy((void *)MAILBOX_DSPBOX_BASE,

&fw_ready_apl, sizeof(fw_ready_apl));

memcpy((void *)(MAILBOX_DSPBOX_BASE + sizeof(fw_ready_apl)),

&sram_window, sizeof(sram_window));

SOC_DCACHE_FLUSH((void *)MAILBOX_DSPBOX_BASE, MAILBOX_DSPBOX_SIZE);

ipc_write(IPC_DIPCIE, 0);

ipc_write(IPC_DIPCI, (0x80000000 | ADSP_IPC_FW_READY));

}

On the host side we use https://github.com/thesofproject/sof-diagnostic-driver which does not use it much.

Linux and the diagnostic driver read this data as part of their IPC init, since mailbox sizes, access etc and IPC HW varies between the SoCs so I dont think there is anything stopping the DT upgrade when time permits.

Add bindings for ADSP mailbox. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

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

Add IPM driver for ADSP. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Add IPM echo sample echoing data over IPM. Add documentation for the sample. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

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>

@finikorg

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

Use for IPC with firmware loader IPM API. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

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

Add by default IPM console for up_squared_adsp board, used for sanity checks. 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 message queue to console in CONFIG_IPM_CONSOLE_IMMEDIATE is not selected. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

For CONFIG_LOG_BACKEND_RB logs are printed to the ring buffer in memory, so special tools are needed to read those logs. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Use line buffer to store characters before sending over IPM. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Update terminal tool to use line buffer read instead of one charcter over IPM. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Update permissions for mmap. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

dcpleung · 2020-08-26T17:00:47Z

??? I thought the driver has already been merged.

finikorg · 2020-08-27T13:50:48Z

??? I thought the driver has already been merged.

I have split this PR to small chunks, IPM driver was merged, remaining are console over IPM and tools.

finikorg · 2020-09-07T07:21:37Z

The PR was split to smaller logical entities and merged upstream. Closing this.

finikorg added the area: IPC Inter-Process Communication label Feb 11, 2020

finikorg requested review from jhedberg and dcpleung February 11, 2020 14:23

finikorg requested review from andrewboie, andyross and nashif as code owners February 11, 2020 14:23

zephyrbot added the area: Samples Samples label Feb 11, 2020

finikorg force-pushed the ipm branch 3 times, most recently from cf0212b to 80f7765 Compare February 12, 2020 13:38

finikorg commented Feb 12, 2020

View reviewed changes

drivers/ipm/ipm_adsp.c Outdated Show resolved Hide resolved

finikorg requested a review from galak as a code owner February 13, 2020 10:35

finikorg force-pushed the ipm branch from 7542af9 to 6bf8fda Compare February 13, 2020 10:41

zephyrbot added the area: Devicetree label Feb 13, 2020

andyross reviewed Feb 13, 2020

View reviewed changes

finikorg requested review from carlescufi and MaureenHelm as code owners February 14, 2020 12:38

zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Feb 14, 2020

finikorg force-pushed the ipm branch 2 times, most recently from 49df5c4 to 30678b8 Compare February 14, 2020 13:10

lgirdwood requested changes Feb 15, 2020

View reviewed changes

finikorg force-pushed the ipm branch from 084d7da to ec4e829 Compare February 19, 2020 10:50

finikorg requested review from ioannisg, joerchan and masz-nordic as code owners February 19, 2020 10:50

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 2, 2020

finikorg mentioned this pull request Jul 3, 2020

sanitycheck: Add serial_pty script command line option #23959

Merged

finikorg force-pushed the ipm branch from 54de900 to bad918a Compare July 3, 2020 13:39

finikorg force-pushed the ipm branch from bad918a to 1cf5ec5 Compare August 4, 2020 12:25

finikorg added 15 commits August 5, 2020 16:35

dts: Add intel ADSP mailbox bindings

eb5db96

Add bindings for ADSP mailbox. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

soc: intel_apl_adsp: Add mailbox to the board

9be7503

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

ipm: ipm_adsp: Add ADSP IPM driver

f33c53b

Add IPM driver for ADSP. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

samples: ipm_echo: Add sample and documentation

e4462cd

Add IPM echo sample echoing data over IPM. Add documentation for the sample. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

console: ipm_console: Add console over IPM

2419dac

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>

CODEOWNERS: Add ipm_adsp driver and ipm_console codeowner.

e14c45f

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

soc: intel_apl_adsp: Use IPM for firmware IPC

8db6a17

Use for IPC with firmware loader IPM API. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

soc: intel_apl_adsp: Add defaults for IPM and IPM console

a94f07c

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

boards: up_squared_adsp: Add IPM and console to the board

c34cd3e

Add by default IPM console for up_squared_adsp board, used for sanity checks. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

boards: up_squared_adsp: Add IPM terminal tool

9fd9fb4

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>

console: ipm_console: Add message queue to console

2d6b58d

Add message queue to console in CONFIG_IPM_CONSOLE_IMMEDIATE is not selected. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

samples: logger: Filter out sanitycheck for ring buffer backend

98a448f

For CONFIG_LOG_BACKEND_RB logs are printed to the ring buffer in memory, so special tools are needed to read those logs. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

console: ipm_console: Use line buffer before sending

b630867

Use line buffer to store characters before sending over IPM. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

boards: up_squared_adsp: update terminal tool

c2ba411

Update terminal tool to use line buffer read instead of one charcter over IPM. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

boards: up_squared_adsp: Update mailbox terminal

0944b46

Update permissions for mmap. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

finikorg force-pushed the ipm branch from 5af3bc9 to 0944b46 Compare August 5, 2020 13:38

finikorg mentioned this pull request Aug 5, 2020

IPM: Add Host-DSP Communication channel driver #27385

Merged

This was referenced Sep 2, 2020

Add IPM console #27804

Merged

sanitycheck: Allow to pass command with argument to ser_pty #27968

Closed

finikorg closed this Sep 7, 2020

finikorg deleted the ipm branch September 9, 2020 08:03

finikorg mentioned this pull request Nov 2, 2020

up_squared_adsp: Sanitycheck can not run test case on Up_Squared_ADSP board #29430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPC: IPM: Add ipm adsp driver & sanitycheck #22724

IPC: IPM: Add ipm adsp driver & sanitycheck #22724

finikorg commented Feb 11, 2020 •

edited

Loading

zephyrbot commented Feb 11, 2020 •

edited

Loading

dcpleung commented Feb 11, 2020

andyross left a comment

andyross Feb 13, 2020

finikorg Mar 23, 2020

lgirdwood Mar 23, 2020

andyross Feb 13, 2020

andyross Feb 13, 2020

finikorg Feb 14, 2020 •

edited

Loading

finikorg Feb 14, 2020

andyross Feb 13, 2020

andyross Feb 13, 2020

finikorg Feb 14, 2020

finikorg Apr 20, 2020

finikorg commented Feb 14, 2020

lgirdwood left a comment

lgirdwood Feb 15, 2020

finikorg Feb 17, 2020

lgirdwood Feb 17, 2020

finikorg Mar 23, 2020

lgirdwood Feb 15, 2020

finikorg Feb 17, 2020 •

edited

Loading

lgirdwood Feb 17, 2020

dcpleung commented Aug 26, 2020

finikorg commented Aug 27, 2020

finikorg commented Sep 7, 2020

	* finishes. If there is deferred processing on the remote side,
	* or you would like to queue outgoing messages and wait on an
	* event/semaphore, you can implement that in a high-level driver

	/*
	* Sends the firmware ready message so the firmware loader can
	* map the host windows.
	*/
	static void send_fw_ready(void)
	{
	memcpy((void *)MAILBOX_DSPBOX_BASE,
	&fw_ready_apl, sizeof(fw_ready_apl));

	memcpy((void *)(MAILBOX_DSPBOX_BASE + sizeof(fw_ready_apl)),
	&sram_window, sizeof(sram_window));

	SOC_DCACHE_FLUSH((void *)MAILBOX_DSPBOX_BASE, MAILBOX_DSPBOX_SIZE);

	ipc_write(IPC_DIPCIE, 0);
	ipc_write(IPC_DIPCI, (0x80000000 \| ADSP_IPC_FW_READY));
	}

IPC: IPM: Add ipm adsp driver & sanitycheck #22724

IPC: IPM: Add ipm adsp driver & sanitycheck #22724

Conversation

finikorg commented Feb 11, 2020 • edited Loading

zephyrbot commented Feb 11, 2020 • edited Loading

dcpleung commented Feb 11, 2020

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finikorg Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finikorg commented Feb 14, 2020

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finikorg Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcpleung commented Aug 26, 2020

finikorg commented Aug 27, 2020

finikorg commented Sep 7, 2020

finikorg commented Feb 11, 2020 •

edited

Loading

zephyrbot commented Feb 11, 2020 •

edited

Loading

finikorg Feb 14, 2020 •

edited

Loading

finikorg Feb 17, 2020 •

edited

Loading