platform: cavs: add config item for IMR restore feature #4386
Conversation
keyonjie
requested review from
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
src/platform/intel/cavs/platform.c
Outdated
There was a problem hiding this comment.
The commit message makes me wonder if
a) IMR restore feature is not supported in hardware for cAVS 1.5?
b) IMR restore feature does not currently work so it's disabled to make forward progress and deal with cAVS 1.5 later?
Which is it @keyonjie?
There was a problem hiding this comment.
The commit message makes me wonder if
a) IMR restore feature is not supported in hardware for cAVS 1.5?
b) IMR restore feature does not currently work so it's disabled to make forward progress and deal with cAVS 1.5 later?Which is it @keyonjie?
I am not 100% sure about this, so I would say b). More information is that I found both runtime PM and S2Idle suspend works on APL, while S3 ([deep]) doesn't. I talked with @lgirdwood and we trend to not spend too much time on experimenting this as no customer is asking for this feature on APL.
There was a problem hiding this comment.
Customers are not asking for this feature on any platform. They just want reliable boot + suspend/resume.
I am not comfortable punting on cAVS 1.5, the boot flow is exactly the same for 1.5..2.5. This is taking a serious risk of missing something that will blow-up later, and having different flows to maintain. Unless IMR restore is disabled on Windows platforms we should have a plan to fix this.
There was a problem hiding this comment.
yes, I will keep following it on APL, the latest debugging makes me believe the value of that IMR region is NOT persisted during S3 cycle on APL, querying this to the hardware team.
There was a problem hiding this comment.
a) IMR restore feature is not supported in hardware for cAVS 1.5?
@plbossart I think we need to say not at the moment and IIUC this was not an issue slowing APL wrt other firmwares.
There was a problem hiding this comment.
@lgirdwood let's not rush this one, I am not ready to merge this, this is one of the scariest part of the flows and we have too many unknowns left and right.
There was a problem hiding this comment.
Not going to merge until after multicore is good.
There was a problem hiding this comment.
More information is that I found both runtime PM and S2Idle suspend works on APL, while S3 ([deep]) doesn't.
I suspect finding this took non-negligible time so please add this as a comment in the source code not to make others pay that time (or more) again.
keyonjie
force-pushed
the
imr_restore
branch
2 times, most recently
from
0251a58 to
7b69ebb
Compare
keyonjie
force-pushed
the
imr_restore
branch
from
7b69ebb to
368bb65
Compare
src/platform/intel/cavs/platform.c
Outdated
There was a problem hiding this comment.
Btw, we should probably have the same prefixes for these boot info flags. Where are they defined ?
|
@lgirdwood just to be clear, this will be merged after the multi-core coherency stuff, yes? I am a bit paranoid with taking too many changes at the same time. |
Yep, we are just getting it in review shape so that we can hit v1.9 after muticore. |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
@keyonjie can we put the whole IMR restore feature in a Kconfig option that is disabled by default on each platform. This way we can merge and allow some platform by platform testing and then enable it after multicore is merged. Just want to give the ADL folks some time to evaluate and test (where they can locally enable it).
Good suggestion @lgirdwood. |
Add kconfig item CAVS_IMR_D3_PERSISTENT to denote if IMR restore feature will be used on an Intel cAVS platform. It will be disabled by default, please only enable it where IMR content is persistent when DSP in D3, as enabling it means we don't need to re-downloading firmware binary to DSP SRAM so fast D3->D0 transition will be supported. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
I don't get the suggestion, you still need the kernel to disable the boot at each transition out of D3. A firmware-only enabling will not do much. |
keyonjie
changed the title
if we disable it from the FW side, we will send FW_READY to host to claim that we don't support this feature, then the kernel side won't try to use it at resuming, the normal re-downloading will happen. @plbossart @lgirdwood just update with the Kconfig solution, no change needed in the Kernel PR. |
|
Jenkins showing CPU freq errors when detecting FW presence on ADL. Unrelated. |
4 participants
platform: cavs: add config item for IMR restore feature
Add kconfig item CAVS_IMR_D3_PERSISTENT to denote if IMR restore feature
will be used on an Intel cAVS platform.
It will be disabled by default, please only enable it where IMR content
is persistent when DSP in D3, as enabling it means we don't need to
re-downloading firmware binary to DSP SRAM so fast D3->D0 transition
will be supported.