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 11:03, Mukunda,Vijendar wrote:
> On 04/07/23 13:57, Pierre-Louis Bossart wrote:
>>
>> On 7/4/23 09:37, Mukunda,Vijendar wrote:
>>> On 04/07/23 12:41, Pierre-Louis Bossart wrote:
>>>> 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.
>>> Both power modes (case 2 & case 4) are different.
>>> The current patch series is targeted to address JD events during system level
>>> suspend/resume scenario, when ClockStop Power mode is selected.i.e case 2
>>> as mentioned above. We're not expecting bus reset during system level resume in
>>> this power mode.
>>> As SDCA interrrupts are not enabled on peripheral side during system level resume,
>>> No JD events are reported.
>>>
>>> Test scenario will be let system level supend gets invoked. During suspend, plug/un-plug jack.
>>> During system level resume, JD status should be reported correctly.
>> This is completely different to the definition of 'clock stop mode'.
>> What you want is to make sure the status is reported correctly when the
>> system resume, not to resume the system using the clock stop exit
>> mechanism at the initiative of the peripheral.
> No.  What we want to convey here is in system level resume for SoundWire manager,
> clock stop exit sequence will be invoked.
> After clock stop exit sequence completed, we expect JD alert should be reported if there is any
> JD event during Sx/D4 state.
> As SDCA interrupts are disabled previously when system level suspend
> is invoked on peripheral side, even after finishing clock stop exit sequence, we are
> no longer observing Jack alerts as SDCA interrupts are disabled.

The existing code in the codec driver disables the IMPL_DEF or SDCA
interrupt in the system suspend routines, no matter what happens on the
manager side.

see e.g. rt711_dev_system_suspend

This patch really has nothing to do with clock stop mode0 proper.

I think what happens is that if there is a new attachment, the
sdw_initialize_slave() helper will re-enable the IMPL_DEF or SDCA
interrupt, and that does not happen if there is no attachment and
re-enumeration.

I will be looking forward to the v2, but as I indicated earlier it might
be a better idea to fix this difference in programming flows in the
clock stop exit code rather in some specific codec drivers.