ICCMAX implementation for FW boot in TGL #2290
Conversation
ranj063
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
libinyang,
lyakh and
plbossart
as code owners
sound/soc/sof/intel/cnl.c
Outdated
There was a problem hiding this comment.
can you clarify your comment @abonislawski ? For TGL it's the same routine but only for core0. the mask is specified in the chip descriptor at line 437
There was a problem hiding this comment.
@plbossart do you mean init core mask? but what if we want to power up a slave core from topology? It will call it too
sof_ops(sdev)->core_power_up(sdev, core_mask)
Not a point for this PR but should be fixed sooner or later
There was a problem hiding this comment.
@abonislawski yes this needs to be fixed for TGL. I can look into it after this PR.
There was a problem hiding this comment.
do we agree that on TGL only core0 can be powered up/down by the driver, all other cores need an IPC?
There was a problem hiding this comment.
Thats right, all other cores need an IPC and FW will power up them
There was a problem hiding this comment.
@plbossart the only change I think would be that this op should check if the requested core mask is 0 and return otherwise. We still need the IPC for other cores.
There was a problem hiding this comment.
@ranj063 yeap, I think it should work then, sof_core_enable() is already sending IPC so just need to return in this op
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Looks mostly good to me, except for the 'ASoC: SOF: Intel: hda: fix error flow during firmware boot' part, which seems unrelated and makes me wonder what we missed.
sound/soc/sof/sof-pci-dev.c
Outdated
There was a problem hiding this comment.
don't we also need this on ICL?
There was a problem hiding this comment.
yes we do. I'm trying to verify if there's a difference in the flow compared to TGL. Will update soon
There was a problem hiding this comment.
@plbossart the flow for ICL is different from TGL. It requires using the ext manifest to know if the FW is configured for LPRO or HPRO. I need to work on the kernel-side parser for this info in the ext manifest before I can implement the flow for ICL. So for now, this change is good for TGL.
sound/soc/sof/intel/cnl.c
Outdated
There was a problem hiding this comment.
can you clarify your comment @abonislawski ? For TGL it's the same routine but only for core0. the mask is specified in the chip descriptor at line 437
sound/soc/sof/intel/hda-loader.c
Outdated
There was a problem hiding this comment.
should you restore the guardband on errors?
There was a problem hiding this comment.
@plbossart stream_prepare would fail only if the iccmax stream hw_params fails and in this case the guardband value wouldnt have been modified at all. So no need to restore here. But if get_stream_with_tag() fails, then we do need to restore which I do below.
ranj063
force-pushed
the
topic/iccmax
branch
5 times, most recently
from
1f15418 to
50cca59
Compare
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
just one cosmetic change request
It should be called VS_LTRP instead. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
For some platforms, the recommended HW sequence for FW boot involves starting a specially crafted capture stream before powering on the DSP cores. Add a helper function to define the minimal recommended stream programming sequence for this stream. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Modify the signature of get_stream_with_tag() to add the direction as an argument to extend it for using with capture streams. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
This will be used for the ICCMAX stream as well. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Define the FW boot sequence for platforms that are recommended to use ICCMAX. This function uses the existing prepare and cleanup functions for creating a specially crafted capture stream before powering up the DSP cores. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Separate the dsp ops for TGL ops to specify the use of ICCMAX FW boot sequence in the run op. All other ops are identical. Also separate the TGL descriptors into a separate file to make it easier to follow. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
|
|
||
| static struct hdac_ext_stream *get_stream_with_tag(struct snd_sof_dev *sdev, | ||
| int tag) | ||
| int tag, int direction) |
There was a problem hiding this comment.
typo in commit message, should be ASoC: SOF: Intel.
Maybe fix in sof-dev-rebase to avoid unnecessary reviews?
4 participants
This PR adds the recommended HW programming sequence for TGL during FW boot. In order to ensure ICCMAX during FW boot, the recommended flow is to start a specially crafted capture stream before initiating FW boot.
Tested on TGL chromebook. I could use some help testing on TGL RVP