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/4/23 08:36, Mukunda,Vijendar wrote:
> On 03/07/23 22:48, Pierre-Louis Bossart wrote:
>>
>> 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.
>>
> We are pointing system level suspend scenario only.
> 
> We have already SoundWire Power off mode - ClockStop Mode will be
> applied followed by bus reset and SoundWire manager will be disabled.
> On parent side, ACP soft reset will be applied and ACP parent driver can
> wake for PME events for Soundwire manager.
> 
> Sx/D4 case, want to avoid re-enumeration when multiple peripherals are connected.
> For Clock Stop + Bus reset (keeping Sdw manager enabled), we can define a new power mode
> on SoundWire manager side.
> 
> Having said that, below are power modes we want to support on our side.
> 
> 1) Idle Mode - Don't stop the clock for SoundWire manager and Soundwire manager is not
> disabled during suspend callbacks.
> 2) ClockStop Mode - Only apply ClockStopMode0 on Soundwire manager side and restore
> the clock when resume sequence is invoked (even in case of system level suspend also)
> 3) ClockStop Mode + Bus reset - Apply ClockStopMode0 when Sound wire manager entering
> runtime suspend state. In system level resume case, apply clock stop exit and bus reset which
> will re-enumerate all peripheral devices over the links.
> 4) Power off mode: apply Clock Stop + SoundWire manager IP power off sequence.
> Parent device(ACP) will be powered off. ACP can wake up for in band wake-up events.
> 
> We expect if someone wants to go with only ClockStop mode, peripheral side also required
> support should be enabled.

I am not following any of this. You wrote both

"
For ClockStop mode SDW manager is not receiving any jack alert when
SoundWire manager device is in D3 state.
"

and

"
Parent device(ACP) will be powered off. ACP can wake up for in band
wake-up events.
"

I can't figure out how you deal with wake-ups and when exactly you
expect jack detection to be functional.