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

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

[shumingf]

From: Shuming Fan <shumingf@realtek.com>

During ClockStop Mode0, peripheral interrupts are disabled.
When system level resume is invoked, Peripheral SDCA interrupts
should be enabled to handle JD events.
Enable SDCA interrupts in resume sequence when ClockStop Mode0 is applied.

Signed-off-by: Shuming Fan <shumingf@realtek.com>
---
 sound/soc/codecs/rt5682-sdw.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
index 67404f45389f..4968a8c0064d 100644
--- a/sound/soc/codecs/rt5682-sdw.c
+++ b/sound/soc/codecs/rt5682-sdw.c
@@ -750,8 +750,15 @@ static int __maybe_unused rt5682_dev_resume(struct device *dev)
 	if (!rt5682->first_hw_init)
 		return 0;
 
-	if (!slave->unattach_request)
+	if (!slave->unattach_request) {
+		if (rt5682->disable_irq == true) {
+			mutex_lock(&rt5682->disable_irq_lock);
+			sdw_write_no_pm(slave, SDW_SCP_INTMASK1, SDW_SCP_INT1_IMPL_DEF);
+			rt5682->disable_irq = false;
+			mutex_unlock(&rt5682->disable_irq_lock);
+		}
 		goto regmap_sync;
+	}
 
 	time = wait_for_completion_timeout(&slave->initialization_complete,
 				msecs_to_jiffies(RT5682_PROBE_TIMEOUT));
-- 
2.40.1

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

[Pierre-Louis Bossart]

On 7/3/23 11:02, shumingf@realtek.com wrote:
> From: Shuming Fan <shumingf@realtek.com>
> 
> 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.

> When system level resume is invoked, Peripheral SDCA interrupts
> should be enabled to handle JD events.

The initial 'when system level resume is invoked' is aligned with my
comment above, this interrupt disabling is only for system suspend.

In addition, I think it's a copy paste here, there is no SDCA support in
RT5682 or the initial RT711.

> Enable SDCA interrupts in resume sequence when ClockStop Mode0 is applied.

same comments for rt711-sdw.c and other codecs which use the same
pattern with the same comment

	/*
	 * prevent new interrupts from being handled after the
	 * deferred work completes and before the parent disables
	 * interrupts on the link
	 */

BTW if this is an issue for system suspend, maybe we should disable the
interrupts on the link in the .prepare stage, that would remove this
step in all codec drivers? The .prepare deals with the parent first,
while .suspend deal with child devices first. The drawback would be that
the codec driver would depend on the manager driver doing the right
thing, which isn't great.

> Signed-off-by: Shuming Fan <shumingf@realtek.com>
> ---
>  sound/soc/codecs/rt5682-sdw.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
> index 67404f45389f..4968a8c0064d 100644
> --- a/sound/soc/codecs/rt5682-sdw.c
> +++ b/sound/soc/codecs/rt5682-sdw.c
> @@ -750,8 +750,15 @@ static int __maybe_unused rt5682_dev_resume(struct device *dev)
>  	if (!rt5682->first_hw_init)
>  		return 0;
>  
> -	if (!slave->unattach_request)
> +	if (!slave->unattach_request) {
> +		if (rt5682->disable_irq == true) {
> +			mutex_lock(&rt5682->disable_irq_lock);
> +			sdw_write_no_pm(slave, SDW_SCP_INTMASK1, SDW_SCP_INT1_IMPL_DEF);
> +			rt5682->disable_irq = false;
> +			mutex_unlock(&rt5682->disable_irq_lock);
> +		}
>  		goto regmap_sync;
> +	}
>  
>  	time = wait_for_completion_timeout(&slave->initialization_complete,
>  				msecs_to_jiffies(RT5682_PROBE_TIMEOUT));

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

[Shuming [范書銘]]

> > 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.
The SdW device will not be re-attached when the system resume.
Therefore, the INT1_IMPL_DEF/SDCA_INTMASK interrupt will need to be enabled when the system resumes.

> > When system level resume is invoked, Peripheral SDCA interrupts should
> > be enabled to handle JD events.
> 
> The initial 'when system level resume is invoked' is aligned with my comment
> above, this interrupt disabling is only for system suspend.
> 
> In addition, I think it's a copy paste here, there is no SDCA support in
> RT5682 or the initial RT711.

Will fix copy paste issue.

> > Enable SDCA interrupts in resume sequence when ClockStop Mode0 is
> applied.
> 
> same comments for rt711-sdw.c and other codecs which use the same pattern
> with the same comment
> 
>         /*
>          * prevent new interrupts from being handled after the
>          * deferred work completes and before the parent disables
>          * interrupts on the link
>          */
> 
> BTW if this is an issue for system suspend, maybe we should disable the
> interrupts on the link in the .prepare stage, that would remove this step in all
> codec drivers? The .prepare deals with the parent first, while .suspend deal
> with child devices first. The drawback would be that the codec driver would
> depend on the manager driver doing the right thing, which isn't great.

I think the codec driver could handle that. The SDCA codec driver already re-enabled the SDCA INT mask when the device is re-attached.

> > Signed-off-by: Shuming Fan <shumingf@realtek.com>
> > ---
> >  sound/soc/codecs/rt5682-sdw.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/rt5682-sdw.c
> > b/sound/soc/codecs/rt5682-sdw.c index 67404f45389f..4968a8c0064d
> > 100644
> > --- a/sound/soc/codecs/rt5682-sdw.c
> > +++ b/sound/soc/codecs/rt5682-sdw.c
> > @@ -750,8 +750,15 @@ static int __maybe_unused
> rt5682_dev_resume(struct device *dev)
> >       if (!rt5682->first_hw_init)
> >               return 0;
> >
> > -     if (!slave->unattach_request)
> > +     if (!slave->unattach_request) {
> > +             if (rt5682->disable_irq == true) {
> > +                     mutex_lock(&rt5682->disable_irq_lock);
> > +                     sdw_write_no_pm(slave, SDW_SCP_INTMASK1,
> SDW_SCP_INT1_IMPL_DEF);
> > +                     rt5682->disable_irq = false;
> > +                     mutex_unlock(&rt5682->disable_irq_lock);
> > +             }
> >               goto regmap_sync;
> > +     }
> >
> >       time =
> wait_for_completion_timeout(&slave->initialization_complete,
> >
> msecs_to_jiffies(RT5682_PROBE_TIMEOUT));
> 
> ------Please consider the environment before printing this e-mail.

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

[Pierre-Louis Bossart]

On 7/3/23 13:11, 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.

> The SdW device will not be re-attached when the system resume.

it will re-attach, and in addition it will lose context because the
manager performs a complete reset of the bus.

So what's needed is to enable the interrupt, no matter what happened in
the suspend transition.

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

[Shuming [范書銘]]

> >>> 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?

> > The SdW device will not be re-attached when the system resume.
> 
> it will re-attach, and in addition it will lose context because the manager
> performs a complete reset of the bus.
> 
> So what's needed is to enable the interrupt, no matter what happened in the
> suspend transition.
> 
> 
> ------Please consider the environment before printing this e-mail.

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

[Mukunda,Vijendar]

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.

Our expectation is when ClockStop Mode is selected, Only ClockStopMode0
should be applied for system level suspend as well.
We are not expecting bus reset.

We have validated these changes on our platform with Clock stop mode.
It's working fine.

>>> The SdW device will not be re-attached when the system resume.
>> it will re-attach, and in addition it will lose context because the manager
>> performs a complete reset of the bus.
>>
>> So what's needed is to enable the interrupt, no matter what happened in the
>> suspend transition.
>>
>>
>> ------Please consider the environment before printing this e-mail.

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

[Pierre-Louis Bossart]

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....

> Our expectation is when ClockStop Mode is selected, Only ClockStopMode0
> should be applied for system level suspend as well.
> We are not expecting bus reset.
> 
> We have validated these changes on our platform with Clock stop mode.
> It's working fine.
> 
>>>> The SdW device will not be re-attached when the system resume.
>>> it will re-attach, and in addition it will lose context because the manager
>>> performs a complete reset of the bus.
>>>
>>> So what's needed is to enable the interrupt, no matter what happened in the
>>> suspend transition.
>>>
>>>
>>> ------Please consider the environment before printing this e-mail.
> 

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

[Mukunda,Vijendar]

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.

>
>> Our expectation is when ClockStop Mode is selected, Only ClockStopMode0
>> should be applied for system level suspend as well.
>> We are not expecting bus reset.
>>
>> We have validated these changes on our platform with Clock stop mode.
>> It's working fine.
>>
>>>>> The SdW device will not be re-attached when the system resume.
>>>> it will re-attach, and in addition it will lose context because the manager
>>>> performs a complete reset of the bus.
>>>>
>>>> So what's needed is to enable the interrupt, no matter what happened in the
>>>> suspend transition.
>>>>
>>>>
>>>> ------Please consider the environment before printing this e-mail.

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

[Pierre-Louis Bossart]

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.

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

[Mukunda,Vijendar]

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.

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

[Pierre-Louis Bossart]

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.

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

[Mukunda,Vijendar]

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.

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

[Pierre-Louis Bossart]

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.

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

[Mukunda,Vijendar]

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.

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

[Pierre-Louis Bossart]

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.

I think we are using 'clock stop' with different meanings. Stopping the
clock in the traditional electrical engineering sense is different to
the MIPI-defined clock stop, where manager and peripheral agrees on when
the clock will stop and how to restart.

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

[Mukunda,Vijendar]

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 current discussion revolves around avoiding bus reset and just enabling SDCA interrupts
when system level resume sequence is invoked on peripheral side for ClockStop Mode0.


> I think we are using 'clock stop' with different meanings. Stopping the
> clock in the traditional electrical engineering sense is different to
> the MIPI-defined clock stop, where manager and peripheral agrees on when
> the clock will stop and how to restart.

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

[Pierre-Louis Bossart]

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.