On Wed, Jan 23, 2019 at 6:16 PM Liam Girdwood liam.r.girdwood@linux.intel.com wrote:

On Wed, 2019-01-23 at 16:42 +0200, Daniel Baluta wrote:

...

Please s/Where/What/ in the title. :))

On Wed, Jan 23, 2019 at 4:41 PM Daniel Baluta daniel.baluta@gmail.com wrote:

...

Hi Pierre/Liam,

In src/include/sof/ipc.h there is:

struct ipc { uint32_t host_msg;» » /* current message from host */ // }

This keeps the message received from IA core. Anyhow, DSP core doesn't seem to use it at all.

Yeah, this is legacy that could be removed.

...

...

Except maybe for some logging:

src/drivers/intel/baytrail/ipc.c: _ipc->host_msg = msg; src/drivers/intel/baytrail/ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/drivers/intel/cavs/ipc.c: _ipc->host_msg = msg; src/drivers/intel/cavs/ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc-

...

host_msg);

src/drivers/intel/cavs/sue-ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/drivers/intel/haswell/ipc.c: _ipc->host_msg = msg; src/drivers/intel/haswell/ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/include/sof/ipc.h: uint32_t host_msg; /* current message from host */

I am asking this because initially I thought the communication between IA core and DSP involves:

• sending cmd using shim layer
• sending actual data using mailbox.

Yep, on the Intel MMIO based architecture we can think of the SHIM as the doorbell and shared memory as the mailbox. The SHIM doorbells can assert IRQs on host and DSP (one doorbell for host -> dsp and the other for DSP -> host)

...

...

But it seems that all the information is exchanged via the mailbox memory. So, going back to my question will host_msg be used in the future?

Yes, this was changed as mailbox was a more architecture neutral method of IPC i.e. it did not depend on passing metadata via special registers.

...

...

If not, I think it is best to remove it because it is confusing.

Feel free to send PR and remove :)

Will do.

...

...

The same happens when DSP tries to send something to IA.

For example it writes msg->header. But this is never used on the other side.

ipc_platform_send_msg

» /* now interrupt host to tell it we have message sent */ » shim_write(SHIM_IPCDL, msg->header); » shim_write(SHIM_IPCDH, SHIM_IPCDH_BUSY);

Also, would be easier for me on the ARM side to only send info via mailbox and notification via what you call shim layer.

Also makes it easier for SPI and virtio too.

It looks like the reply from DSP sent via shim register it used on the IA core side, though.

On DSP:

void ipc_platform_send_msg(struct ipc *ipc) » /* now interrupt host to tell it we have message sent */ » shim_write(SHIM_IPCDL, msg->header); » shim_write(SHIM_IPCDH, SHIM_IPCDH_BUSY);

On IA core:

byt_irq_thread ipcx = snd_sof_dsp_read64(sdev, BYT_DSP_BAR, SHIM_IPCX); snd_sof_ipc_reply(sdev, ipcx)

So, in order to find the initial message for which this reply is intended we transmit the information inside the SHIM_IPCX register.

Will try to see if we can move the id inside the mailbox area, and only keep the SHIM as a true doorbell.

Thanks a lot for explanations Liam.

Daniel.

On Wed, Jan 23, 2019 at 8:32 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:

...

...

...

But it seems that all the information is exchanged via the mailbox memory. So, going back to my question will host_msg be used in the future?

Yes, this was changed as mailbox was a more architecture neutral method of IPC i.e. it did not depend on passing metadata via special registers.

we still have a case for Baytrail (also reported by Daniel) where the IPC seems to mix commands and doorbell status:

static int byt_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg) { u64 cmd = msg->header;

 /* send the message */
 sof_mailbox_write(sdev, sdev->host_box.offset, msg->msg_data,
           msg->msg_size);
 snd_sof_dsp_write64(sdev, BYT_DSP_BAR, SHIM_IPCX,
             cmd | SHIM_BYT_IPCX_BUSY); <<< WTH?

 return 0;

There is also: * hda_dsp_ipc_send_msg * cnl_ipc_send_msg

It looks like cmd could be removed from all of them. Will send a patch later if anyone agrees that ORing cmd inside the IPCX register is not needed.

On Wed, Jan 23, 2019 at 11:10 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:

On 1/23/19 12:50 PM, Daniel Baluta wrote:

...

On Wed, Jan 23, 2019 at 8:32 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:

...

...

...

...

But it seems that all the information is exchanged via the mailbox memory. So, going back to my question will host_msg be used in the future?

Yes, this was changed as mailbox was a more architecture neutral method of IPC i.e. it did not depend on passing metadata via special registers.

we still have a case for Baytrail (also reported by Daniel) where the IPC seems to mix commands and doorbell status:

static int byt_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg) { u64 cmd = msg->header;

  /* send the message */
  sof_mailbox_write(sdev, sdev->host_box.offset, msg->msg_data,
            msg->msg_size);
  snd_sof_dsp_write64(sdev, BYT_DSP_BAR, SHIM_IPCX,
              cmd | SHIM_BYT_IPCX_BUSY); <<< WTH?

  return 0;

There is also:

• hda_dsp_ipc_send_msg
• cnl_ipc_send_msg

It looks like cmd could be removed from all of them. Will send a patch later if anyone agrees that ORing cmd inside the IPCX register is not needed.

I wonder if this doesn't hide a much larger issue with even more code refactoring needed.

see e.g. for PCM, we do this:

 /* set IPC PCM parameters */
 pcm.hdr.size = sizeof(pcm);
 pcm.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_PARAMS;

[...] /* send IPC to the DSP */ ret = sof_ipc_tx_message(sdev->ipc, pcm.hdr.cmd, &pcm, sizeof(pcm), &ipc_params_reply, sizeof(ipc_params_reply));

I just don't get why this second "header" argument is needed and what it is used for in the ipc.c file. There is all sorts of code in there that deals with headers, message_id, etc, and all of that looks like needless complexity and layers of crud that needs to do.

If we really want to have a simpler communication scheme everything should be in-band and part of the same data structure, without duplication or shortcuts.

Liam?

Yes, would be nice to hear from Liam the history of these changes.

It looks to me that at some point the code switched from using SHIM to mailbox but code cleanup was never finished.

On the TX side this switched looks complete (modulo the cleanup).

But on the RX side there is still SHIM registers used for getting the replies :(.

Thanks, Daniel.