Re: [PATCH 1/9] ASoC: SOF: imx: introduce more common structures and functions

[Laurentiu Mihalcea]

(snip)

>>>> +#include <linux/pm_domain.h>
>>>>  #include <sound/sof/xtensa.h>
>>>> +
>>>>  #include "../ops.h"
>>>>
>>>>  #include "imx-common.h"
>>>> @@ -74,5 +79,417 @@ void imx8_dump(struct snd_sof_dev *sdev, u32 flags)
>>>>  }
>>>>  EXPORT_SYMBOL(imx8_dump);
>>>>
>>>> +static void imx_handle_reply(struct imx_dsp_ipc *ipc)
>>>> +{
>>>> +	unsigned long flags;
>>>> +	struct snd_sof_dev *sdev = imx_dsp_get_data(ipc);
>>>> +
>>>> +	spin_lock_irqsave(&sdev->ipc_lock, flags);
>>>> +	snd_sof_ipc_process_reply(sdev, 0);
>>>> +	spin_unlock_irqrestore(&sdev->ipc_lock, flags);
>>> Are you sure have to use spin_lock?
>> not sure, this definition was taken from previous drivers. I'd say keep it for now
>> as removing it would require some more testing which will take some time.
>>
> Change lock type need seperate patch. analyze is more important then test.
> It is really hard to hit race condition at normal user case.
>
> You can check if any irq handle use ipc_lock? If irq handle already defer
> to thread irq, mutex most likely is enough.
>
> Frank

ACK

>
>> (snip)
>>
>>>> +static int imx_suspend(struct snd_sof_dev *sdev, unsigned int target_state)
>>>> +{
>>>> +	int ret;
>>>> +	const struct sof_dsp_power_state target_power_state = {
>>>> +		.state = target_state,
>>>> +	};
>>>> +
>>>> +	if (!pm_runtime_suspended(sdev->dev)) {
>>>> +		ret = imx_common_suspend(sdev);
>>>> +		if (ret < 0) {
>>>> +			dev_err(sdev->dev, "failed to common suspend: %d\n", ret);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return snd_sof_dsp_set_power_state(sdev, &target_power_state);
>>> does pm_runtime_force_suspend()/pm_runtime_force_resume() work?
>> no, these functions are not called directly by the PM core, but rather by the SOF core.
>> Using the proposed functions would result in the SOF core PM functions (i.e: sof_resume/sof_suspend)
>> being called twice in the case of system suspend/resume, which is wrong.
> Does SOC core should check it? I don't reject this change, but check
> run time pm status in suspend often cause some bugs.
>
> Frank

No, IMO this sequence is chip-specific so it shouldn't be handled by the core.