[dbaluta]

We use shim registers only to notify the other
side that a message was sent. The actual information
for the message is transmitted via mailbox.

cmd information inside shim register is not used by
the DSP, so we remove it to avoid confusion.

Signed-off-by: Daniel Baluta daniel.baluta@gmail.com

[keyonjie]

Why you are saying "cmd information inside shim register is not used by the DSP"? if the DSP you mentioned is SOF firmware on Intel Audio DSP, I think you are wrong here.

[plbossart]

Why you are saying "cmd information inside shim register is not used by the DSP"? if the DSP you mentioned is SOF firmware on Intel Audio DSP, I think you are wrong here.

The explanations from @lgirdwood is that the SHIM registers are only used for doorbell operation (are you ready, here's data, etc)., all the data goes over the memory windows. So if you are using the SHIM register to pass any information, you have information that's passed twice through different mechanisms, and this model doesn't scale to other platforms. Not to mention that ORing a software-defined command with a hardware register is a big no-no in general, if a hardware dude adds a chicken bit your solution is DOA.

[keyonjie]

Why you are saying "cmd information inside shim register is not used by the DSP"? if the DSP you mentioned is SOF firmware on Intel Audio DSP, I think you are wrong here.

The explanations from @lgirdwood is that the SHIM registers are only used for doorbell operation (are you ready, here's data, etc)., all the data goes over the memory windows. So if you are using the SHIM register to pass any information, you have information that's passed twice through different mechanisms, and this model doesn't scale to other platforms. Not to mention that ORing a software-defined command with a hardware register is a big no-no in general, if a hardware dude adds a chicken bit your solution is DOA.

I actually like the solution only use SHIM registers as doorbell and send IPC cmd/data via mailbox(memory window), what I concern here is that we need to change this in both FW and driver at the same time, otherwise, platforms may be blocked if this single PR is merged.

[plbossart]

Are you saying this would result in a backwards compatibility issue if we clean this up? Gah.

[keyonjie]

Sorry, just went through the FW side, we already discarded use SHIM message for host initiated IPC, but still writing message to SHIM for FW initiated IPC, so this PR(change the former ones only) should be fine except that FW will printing some logs with msg read from SHIM(e.g. trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg);).

[RanderWang]

I also discussed it with Liam about this issue. For HSW or BDW, we don't send cmd via SHIM register because the cmd can't be included by SHIM register. Now, cmd is read from mailbox, so it is good to remove cmd in SHIM register on all platforms. But we need to refine trace log in FW.

And last time I said some one in community would send a patch to remove cmd in SHIM, now it comes true.

[RanderWang]

And more, we need to do more refinement in kernel.
In our IRQ thread, we get cmd from SHIM register to find msg from reply_list.
Actually, cmd is always zero, and always the first msg (and the only one msg) in reply_list is found.
So we need to refine the algorithm to process reply_msg.

[keyonjie]

And more, we need to do more refinement in kernel.
In our IRQ thread, we get cmd from SHIM register to find msg from reply_list.
Actually, cmd is always zero, and always the first msg (and the only one msg) in reply_list is found.
So we need to refine the algorithm to process reply_msg.

yes, that's the issue #516, after this SHIM register cleanup, we can fix #516 more easily. something like this:
dc4e87b

[dbaluta]

Sorry, just went through the FW side, we already discarded use SHIM message for host initiated IPC, but still writing message to SHIM for FW initiated IPC, so this PR(change the former ones only) should be fine except that FW will printing some logs with msg read from SHIM(e.g. trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg);).

@keyonjie Those logs are removed with thesofproject/sof#877

[dbaluta]

Reopened this because I've "accidentally" deleted my local branch. @keyonjie can you have a look?

[wenqingfu]

Reopened this because I've "accidentally" deleted my local branch. @keyonjie can you have a look?

Sounds like an "approve" from @RanderWang and @keyonjie. You probably won't see a feedback from them shortly for the Chinese new year holiday break. Add @lyakh for his information anyway

[lyakh]

FWIW this also matches the SPI use-case better, where there's only one data path between the host and the DSP and the doorbell function is implemented by GPIO(s).

[plbossart]

Looks good to me, but I want to test it on Baytrail/Up2 before merging. will do it later today.

[dbaluta]

@plbossart thanks for doing that!

With this patch, there is no info passed between AP -> DSP using the SHIM registers. The next topic would be why do we need to pass info using SHIM registers in the direction DSP -> AP.

This will greatly help my implementation on i.MX. I will need to take some time to investigate the RX side (looking from AP).

[plbossart]

@dbaluta i don't see any issues with APL but I see odd results on CHT that I don't see without this PR, e.g.

root@plb-T100HAN:~/kmod_scripts# speaker-test -Dhw:0,0 -c2 -r48000 -t tone

speaker-test 1.1.6

Playback device is hw:0,0
Stream parameters are 48000Hz, S16_LE, 2 channels
Playback open error: -16,Device or resource busy
root@plb-T100HAN:~/kmod_scripts# speaker-test -Dhw:0,0 -c2 -r48000 -t tone

speaker-test 1.1.6

Playback device is hw:0,0
Stream parameters are 48000Hz, S16_LE, 2 channels
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 384 to 16320
Period size range from 192 to 4032
Using max buffer size 16320
Periods = 4

I would really prefer it if we waited for the rest of the team to double-check the results.

[dbaluta]

@plbossart sure! Thanks for trying it.

[plbossart]

@lyakh are you planning to include this change from @dbaluta in your IPC work? If yes maybe we can close this?

[plbossart]

@lyakh friendly reminder that this change probably needs to be folded into your IPC work.

[ranj063]

@plbossart @dbaluta we've waited long enough to decide the fate of this PR. But with the FW counterpart proposing to use the doorbell registers for sending the IPC payload, I think it makes sense to make the proper change now.
thesofproject/sof@851ff9f

If I read the FW commit correctly, if the cmd is added to the doorbell register, it means we want to send the IPC payload via the register and not the shared memory.

[plbossart]

@ranj063 the suggested change is only for cAVS platforms. BYT cannot use the IPC registers for communication, we have to use the mailbox

[ranj063]

@ranj063 the suggested change is only for cAVS platforms. BYT cannot use the IPC registers for communication, we have to use the mailbox

@plbossart then it makes sense to remove the cmd by default as suggested in this PR, isnt it? It should be used only when needed explicitly

[plbossart]

@plbossart <https://github.com/plbossart> then it makes sense to remove the cmd by default as suggested in this PR, isnt it? It should be used only when needed explicitly

yep.

[dbaluta]

@ranj063 @plbossart agree.

[RanderWang]

APPROVE