ASoC: SOF: Intel: hda: modify core_power_up/down op

[ranj063]

Modify the core_power_up/down op for HDA platforms to restrict
the core_mask to the ones allowed by chip->cores_mask. This is needed
because on some HDA platforms not all cores can be powered up/down
by the host and this must be handled internally in the FW.

Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

[lyakh]

this is interesting. So, chip->cores_mask is not all DSP cores, but only those, power managed by the host? .cores_num too? Looks like .cores_num isn't used at all. If anything it might be difficult to follow, IMHO we need at least comments in struct sof_intel_dsp_desc

[plbossart]

the number of cores could be useful in future updates to verify that e.g. if we want a pipeline on core 3, well do we actually have a core3?

That said I wonder if we shouldn't use 'physical_cores' and 'power_managed_cores' to explain what this really means. cores_mask looks like number of cores, it really doesn't help understand the nuance about pm differences.

[lyakh]

yes, would be good to make this a bit clearer

[plbossart]

question and comment below. Thanks @ranj063

[plbossart]

I am also struggling a bit here. If e.g. a topology needs a pipeline on core 2, then how will core 2 be turned on? We're missing the IPC to the firmware, aren't we?

While I am at it, we should remove all the mask macros and use BIT() and GENMASK() for clarity. The use of HDA_DSP_CORE_MASK() is just not useful and prevents us from using GENMASK

[ranj063]

@plbossart we do 2 things during topology load today: Power up the cores and send the IPC

linux/sound/soc/sof/topology.c

Line 1292 in 06accba

static int sof_core_enable(struct snd_sof_dev *sdev, int core)

What we were missing was that we would unnecessarily set the SPA bits for the cores that cant possibly be powered on fom the host.

[plbossart]

but that comment

linux/sound/soc/sof/topology.c

Line 1317 in 06accba

/* Now notify DSP that the core has been powered up */

is not longer correct then, you change the meaning of the IPC from telling the DSP has been turned on to requesting the firmware to turn a core on.

[ranj063]

right. let me add some more cleanups and comments to make this change a bit more intuitive

[plbossart]

the number of cores could be useful in future updates to verify that e.g. if we want a pipeline on core 3, well do we actually have a core3?

That said I wonder if we shouldn't use 'physical_cores' and 'power_managed_cores' to explain what this really means. cores_mask looks like number of cores, it really doesn't help understand the nuance about pm differences.

[plbossart]

@ranj063 do you mind updating this PR, there were only cosmetic comments from Guennadi and I, we should send this upstream when the merge window opens.

[ranj063]

@ranj063 do you mind updating this PR, there were only cosmetic comments from Guennadi and I, we should send this upstream when the merge window opens.

@plbossart ack. I can do this by Monday.

[RanderWang]

Looks good for me. Only one concern about "ret1"

[RanderWang]

LGTM

[keyonjie]

Thanks for the refining here, @ranj063 it looks almost good to me, but how about the cores disable sequence, we might need similar logic like send IPC to FW first and then power down host_managed_cores and then update the enabled_cores_mask?

[ranj063]

Thanks for the refining here, @ranj063 it looks almost good to me, but how about the cores disable sequence, we might need similar logic like send IPC to FW first and then power down host_managed_cores and then update the enabled_cores_mask?

@keyonjie you have a point but that will have to be another PR. Unfortunately during widget unload, we cant just power down the core its pipeline is scheduled on. We need some kind of refcounting to make sure there isnt anything else using it.

[keyonjie]

Thanks for the refining here, @ranj063 it looks almost good to me, but how about the cores disable sequence, we might need similar logic like send IPC to FW first and then power down host_managed_cores and then update the enabled_cores_mask?

@keyonjie you have a point but that will have to be another PR. Unfortunately during widget unload, we cant just power down the core its pipeline is scheduled on. We need some kind of refcounting to make sure there isnt anything else using it.

Oh, yes, that is what my previous PR #888 did. So at this point, it looks good to me.