Re: [PATCH 1/5] ASoC: rt5682-sdw: fix for JD event handling in ClockStop Mode0

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 7/3/23 17:18, Mukunda,Vijendar wrote:
> On 03/07/23 20:15, Pierre-Louis Bossart wrote:
>>
>> On 7/3/23 16:46, Mukunda,Vijendar wrote:
>>> On 03/07/23 19:50, Pierre-Louis Bossart wrote:
>>>> On 7/3/23 15:31, Mukunda,Vijendar wrote:
>>>>> On 03/07/23 18:30, Shuming [范書銘] wrote:
>>>>>>>>>> During ClockStop Mode0, peripheral interrupts are disabled.
>>>>>>>>> I can see that the interrupts are disabled in
>>>>>>>>> rt5682_dev_system_suspend(), which is NOT a mode where the clock stop
>>>>>>>>> is used... I don't think this commit message is correct.
>>>>>>>>>
>>>>>>>>> The IMPL_DEF interrupt which is used for jack detection is not
>>>>>>>>> disabled at all during any clock stop mode, and it shouldn't
>>>>>>>>> otherwise that would break the jack detection.
>>>>>>>> You are right. The commit message is wrong and not clear.
>>>>>>>> The situation is that the manager driver uses the clock stop mode0 to do
>>>>>>> system suspension.
>>>>>>>
>>>>>>> No it does not. The clock stop is ONLY used for pm_runtime, never for system
>>>>>>> suspend. We cannot go to system suspend with the link in clock-stop mode,
>>>>>>> that will create lots of issues, that's why we perform a full pm_runtime resume
>>>>>>> in the .prepare stage.
>>>>>> OK, I got your point. Thanks. However, this issue reported by AMD. 
>>>>>> The AMD platform validated system level pm and runtime pm ops with the different modes.
>>>>>>
>>>>>> Hi Vijendar,
>>>>>> Do you have any comments?
>>>>> On AMD platforms, we are supporting two power modes.
>>>>> 1) Bus reset mode
>>>>> 2) Clock Stop Mode
>>>>>
>>>>> In Bus reset mode, bus will re-enumerate the peripheral devices
>>>>> whereas in ClockStop Mode, applying ClockStop Mode0
>>>>> in both pm ops (runtime pm ops and system level pm ops).
>>>>>
>>>>> Currently, SDCA interrupts are disabled on peripheral side, when system level
>>>>> suspend is invoked.
>>>>> For ClockStop mode SDW manager is not receiving any jack alert when
>>>>> SoundWire manager device is in D3 state.
>>>> That was precisely the point of clock stop mode: a peripheral can
>>>> restart the system even when it's in lower-power mode.
>>>>
>>>> If there's no means to let a peripheral restart, the only benefit is
>>>> maybe to skip the enumeration time. That's not what the spec intended....
>>> As per our understanding, you are pointing to ClockStopMode1.
>>> ClockStopMode1 requires re-enumeration as peripherals move
>>> to unattached state.
>>> We have cross-checked ClockStopMode0 description in spec.
>>> It doesn't specify about peripheral device state as Unattached.
>>> We are referring here "ClockStopMode0" only.
>> No I was describing the difference between the 'Bus reset mode' and the
>> 'clock stop mode' on the manager side.
>>
>> There's also nothing in the spec preventing the manager from doing a
>> reset at any time, including after exiting the clock mode0 stop.
>>
>>
> Partly I agree.  As per our understanding, If any of the peripherals lost's sync,
> and re-enumeration is required.
> If continuous parity errors/bus clash conditions are reported over the link,
> Sdw Manager bus reset sequence should be invoked. This is a different scenario.
> Both the scenarios are asynchronous.
>  
> Going with Spec definition for ClockStopMode0, as it's Imp defined for
> SoundWire Manager, want to stick to Clockstop when D3 call is invoked
> and restore the clock when D0 call is invoked for our platforms.

The problem is that 'D3' can be used for two separate scenarios
- S0/D3: that's pm_runtime suspend
- Sx/D4: that's system suspend
It's very unclear what the benefit of clock stop mode would be for the
latter case.