Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1621020 > unrolled thread

Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions m18063 <Claudiu.Beznea@microchip.com> - 2017-04-11 10:30 +0200
    Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-04-11 11:00 +0200
      Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions m18063 <Claudiu.Beznea@microchip.com> - 2017-04-11 11:50 +0200
        Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-04-11 12:00 +0200

#1621020 — Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions

Hi Boris,

On 10.04.2017 17:35, Boris Brezillon wrote:
> On Mon, 10 Apr 2017 17:20:20 +0300
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
>> Implement suspend and resume power management specific
>> function to allow PWM controller to correctly suspend
>> and resume.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/pwm/pwm-atmel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>> index 530d7dc..75177c6 100644
>> --- a/drivers/pwm/pwm-atmel.c
>> +++ b/drivers/pwm/pwm-atmel.c
>> @@ -58,6 +58,8 @@
>>  #define PWM_MAX_PRD		0xFFFF
>>  #define PRD_MAX_PRES		10
>>  
>> +#define PWM_MAX_CH_NUM		(4)
>> +
>>  struct atmel_pwm_registers {
>>  	u8 period;
>>  	u8 period_upd;
>> @@ -65,11 +67,18 @@ struct atmel_pwm_registers {
>>  	u8 duty_upd;
>>  };
>>  
>> +struct atmel_pwm_pm_ctx {
>> +	u32 cmr;
>> +	u32 cdty;
>> +	u32 cprd;
>> +};
>> +
>>  struct atmel_pwm_chip {
>>  	struct pwm_chip chip;
>>  	struct clk *clk;
>>  	void __iomem *base;
>>  	const struct atmel_pwm_registers *regs;
>> +	struct atmel_pwm_pm_ctx ctx[PWM_MAX_CH_NUM];
> 
> Hm, I'm pretty sure you can rely on the current PWM state and call
> atmel_pwm_apply() at resume time instead of doing that. See what I did
> here [1].

I agree with the approach you propose but the thing is the atmel_pwm_apply()
take care of both, current PWM state and the new state received as argument
in order to change only duty factor without disabling the PWM channel (if
channel is enabled) and then returns. Changing PWM duty and period and polarity
in the same step without disabling + enabling the PWM channel (with atomic
approach) may lead to intermediary unwanted output waveforms (the IP doesn't
support this for ordinary PWM channels). To take advantage of atmel_pwm_apply()
(in the formit is today) in resume() hook might need to first call it to disable
channel and then to enable it. Or atmel_pwm_apply() should be changed to also
disable + enable the channel when user changes the duty factor at runtime.

> 
> Thierry, maybe it's time to start thinking about a generic solution to
> save/restore PWM states.
> 
>>  
>>  	unsigned int updated_pwms;
>>  	/* ISR is cleared when read, ensure only one thread does that */
>> @@ -333,6 +342,77 @@ atmel_pwm_get_driver_data(struct platform_device *pdev)
>>  	return (struct atmel_pwm_registers *)id->driver_data;
>>  }
>>  
>> +#ifdef CONFIG_PM_SLEEP
>> +static int atmel_pwm_suspend(struct device *dev)
>> +{
>> +	struct atmel_pwm_chip *atmel_pwm = dev_get_drvdata(dev);
>> +	struct pwm_device *pwm = atmel_pwm->chip.pwms;
>> +	int i;
>> +	bool disable_clk = false;
>> +
>> +	for (i = 0; i < atmel_pwm->chip.npwm; i++, pwm++) {
>> +		if (!pwm_is_enabled(pwm))
>> +			continue;
>> +
>> +		disable_clk = true;
>> +		atmel_pwm->ctx[i].cdty =
>> +			atmel_pwm_ch_readl(atmel_pwm, i,
>> +					   atmel_pwm->regs->duty);
>> +		atmel_pwm->ctx[i].cprd =
>> +			atmel_pwm_ch_readl(atmel_pwm, i,
>> +					   atmel_pwm->regs->period);
>> +		atmel_pwm->ctx[i].cmr =
>> +			atmel_pwm_ch_readl(atmel_pwm, i, PWM_CMR);
>> +
>> +		atmel_pwm_disable(&atmel_pwm->chip, pwm, false);
>> +	}
>> +
>> +	if (disable_clk)
>> +		clk_disable(atmel_pwm->clk);
> 
> I'm not so sure we want to disable the PWM and the PWM chip clk when
> entering suspend. What if the PWM is driving a critical device (like a
> regulator) that has to stay enabled in suspend?
> Shouldn't we delegate this responsibility to the PWM user?
It is a good point.
> 
> [1]http://patchwork.ozlabs.org/patch/734306/
> 

[toc] | [next] | [standalone]

#1621042

On Tue, 11 Apr 2017 11:22:39 +0300
m18063 <Claudiu.Beznea@microchip.com> wrote:

> Hi Boris,
> 
> On 10.04.2017 17:35, Boris Brezillon wrote:
> > On Mon, 10 Apr 2017 17:20:20 +0300
> > Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> >   
> >> Implement suspend and resume power management specific
> >> function to allow PWM controller to correctly suspend
> >> and resume.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >> ---
> >>  drivers/pwm/pwm-atmel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 81 insertions(+)
> >>
> >> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> >> index 530d7dc..75177c6 100644
> >> --- a/drivers/pwm/pwm-atmel.c
> >> +++ b/drivers/pwm/pwm-atmel.c
> >> @@ -58,6 +58,8 @@
> >>  #define PWM_MAX_PRD		0xFFFF
> >>  #define PRD_MAX_PRES		10
> >>  
> >> +#define PWM_MAX_CH_NUM		(4)
> >> +
> >>  struct atmel_pwm_registers {
> >>  	u8 period;
> >>  	u8 period_upd;
> >> @@ -65,11 +67,18 @@ struct atmel_pwm_registers {
> >>  	u8 duty_upd;
> >>  };
> >>  
> >> +struct atmel_pwm_pm_ctx {
> >> +	u32 cmr;
> >> +	u32 cdty;
> >> +	u32 cprd;
> >> +};
> >> +
> >>  struct atmel_pwm_chip {
> >>  	struct pwm_chip chip;
> >>  	struct clk *clk;
> >>  	void __iomem *base;
> >>  	const struct atmel_pwm_registers *regs;
> >> +	struct atmel_pwm_pm_ctx ctx[PWM_MAX_CH_NUM];  
> > 
> > Hm, I'm pretty sure you can rely on the current PWM state and call
> > atmel_pwm_apply() at resume time instead of doing that. See what I did
> > here [1].  
> 
> I agree with the approach you propose but the thing is the atmel_pwm_apply()
> take care of both, current PWM state and the new state received as argument
> in order to change only duty factor without disabling the PWM channel (if
> channel is enabled) and then returns. Changing PWM duty and period and polarity
> in the same step without disabling + enabling the PWM channel (with atomic
> approach) may lead to intermediary unwanted output waveforms (the IP doesn't
> support this for ordinary PWM channels). To take advantage of atmel_pwm_apply()
> (in the formit is today) in resume() hook might need to first call it to disable
> channel and then to enable it. Or atmel_pwm_apply() should be changed to also
> disable + enable the channel when user changes the duty factor at runtime.

Nope. Just save the state at suspend time, implement ->get_state() and
use it to retrieve the real PWM state when resuming before restoring
the state you saved during suspend.
But anyway, as Thierry explained, I'm not sure we should take the
're-apply PWM state' action here. It's probably better to leave this
decision to the PWM user.

[toc] | [prev] | [next] | [standalone]

#1621099

On 11.04.2017 11:56, Boris Brezillon wrote:
> On Tue, 11 Apr 2017 11:22:39 +0300
> m18063 <Claudiu.Beznea@microchip.com> wrote:
> 
>> Hi Boris,
>>
>> On 10.04.2017 17:35, Boris Brezillon wrote:
>>> On Mon, 10 Apr 2017 17:20:20 +0300
>>> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
>>>   
>>>> Implement suspend and resume power management specific
>>>> function to allow PWM controller to correctly suspend
>>>> and resume.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> ---
>>>>  drivers/pwm/pwm-atmel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 81 insertions(+)
>>>>
>>>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>>>> index 530d7dc..75177c6 100644
>>>> --- a/drivers/pwm/pwm-atmel.c
>>>> +++ b/drivers/pwm/pwm-atmel.c
>>>> @@ -58,6 +58,8 @@
>>>>  #define PWM_MAX_PRD		0xFFFF
>>>>  #define PRD_MAX_PRES		10
>>>>  
>>>> +#define PWM_MAX_CH_NUM		(4)
>>>> +
>>>>  struct atmel_pwm_registers {
>>>>  	u8 period;
>>>>  	u8 period_upd;
>>>> @@ -65,11 +67,18 @@ struct atmel_pwm_registers {
>>>>  	u8 duty_upd;
>>>>  };
>>>>  
>>>> +struct atmel_pwm_pm_ctx {
>>>> +	u32 cmr;
>>>> +	u32 cdty;
>>>> +	u32 cprd;
>>>> +};
>>>> +
>>>>  struct atmel_pwm_chip {
>>>>  	struct pwm_chip chip;
>>>>  	struct clk *clk;
>>>>  	void __iomem *base;
>>>>  	const struct atmel_pwm_registers *regs;
>>>> +	struct atmel_pwm_pm_ctx ctx[PWM_MAX_CH_NUM];  
>>>
>>> Hm, I'm pretty sure you can rely on the current PWM state and call
>>> atmel_pwm_apply() at resume time instead of doing that. See what I did
>>> here [1].  
>>
>> I agree with the approach you propose but the thing is the atmel_pwm_apply()
>> take care of both, current PWM state and the new state received as argument
>> in order to change only duty factor without disabling the PWM channel (if
>> channel is enabled) and then returns. Changing PWM duty and period and polarity
>> in the same step without disabling + enabling the PWM channel (with atomic
>> approach) may lead to intermediary unwanted output waveforms (the IP doesn't
>> support this for ordinary PWM channels). To take advantage of atmel_pwm_apply()
>> (in the formit is today) in resume() hook might need to first call it to disable
>> channel and then to enable it. Or atmel_pwm_apply() should be changed to also
>> disable + enable the channel when user changes the duty factor at runtime.
> 
> Nope. Just save the state at suspend time, implement ->get_state() and
> use it to retrieve the real PWM state when resuming before restoring
> the state you saved during suspend.
Ok.
> But anyway, as Thierry explained, I'm not sure we should take the
> 're-apply PWM state' action here. It's probably better to leave this
> decision to the PWM user. 
Do you thinks we should proceed with restoring the registers behind
the re-apply as other drivers does at this moment?
> 

[toc] | [prev] | [next] | [standalone]

#1621107

On Tue, 11 Apr 2017 12:41:59 +0300
m18063 <Claudiu.Beznea@microchip.com> wrote:

> On 11.04.2017 11:56, Boris Brezillon wrote:
> > On Tue, 11 Apr 2017 11:22:39 +0300
> > m18063 <Claudiu.Beznea@microchip.com> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 10.04.2017 17:35, Boris Brezillon wrote:  
> >>> On Mon, 10 Apr 2017 17:20:20 +0300
> >>> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> >>>     
> >>>> Implement suspend and resume power management specific
> >>>> function to allow PWM controller to correctly suspend
> >>>> and resume.
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >>>> ---
> >>>>  drivers/pwm/pwm-atmel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 81 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> >>>> index 530d7dc..75177c6 100644
> >>>> --- a/drivers/pwm/pwm-atmel.c
> >>>> +++ b/drivers/pwm/pwm-atmel.c
> >>>> @@ -58,6 +58,8 @@
> >>>>  #define PWM_MAX_PRD		0xFFFF
> >>>>  #define PRD_MAX_PRES		10
> >>>>  
> >>>> +#define PWM_MAX_CH_NUM		(4)
> >>>> +
> >>>>  struct atmel_pwm_registers {
> >>>>  	u8 period;
> >>>>  	u8 period_upd;
> >>>> @@ -65,11 +67,18 @@ struct atmel_pwm_registers {
> >>>>  	u8 duty_upd;
> >>>>  };
> >>>>  
> >>>> +struct atmel_pwm_pm_ctx {
> >>>> +	u32 cmr;
> >>>> +	u32 cdty;
> >>>> +	u32 cprd;
> >>>> +};
> >>>> +
> >>>>  struct atmel_pwm_chip {
> >>>>  	struct pwm_chip chip;
> >>>>  	struct clk *clk;
> >>>>  	void __iomem *base;
> >>>>  	const struct atmel_pwm_registers *regs;
> >>>> +	struct atmel_pwm_pm_ctx ctx[PWM_MAX_CH_NUM];    
> >>>
> >>> Hm, I'm pretty sure you can rely on the current PWM state and call
> >>> atmel_pwm_apply() at resume time instead of doing that. See what I did
> >>> here [1].    
> >>
> >> I agree with the approach you propose but the thing is the atmel_pwm_apply()
> >> take care of both, current PWM state and the new state received as argument
> >> in order to change only duty factor without disabling the PWM channel (if
> >> channel is enabled) and then returns. Changing PWM duty and period and polarity
> >> in the same step without disabling + enabling the PWM channel (with atomic
> >> approach) may lead to intermediary unwanted output waveforms (the IP doesn't
> >> support this for ordinary PWM channels). To take advantage of atmel_pwm_apply()
> >> (in the formit is today) in resume() hook might need to first call it to disable
> >> channel and then to enable it. Or atmel_pwm_apply() should be changed to also
> >> disable + enable the channel when user changes the duty factor at runtime.  
> > 
> > Nope. Just save the state at suspend time, implement ->get_state() and
> > use it to retrieve the real PWM state when resuming before restoring
> > the state you saved during suspend.  
> Ok.
> > But anyway, as Thierry explained, I'm not sure we should take the
> > 're-apply PWM state' action here. It's probably better to leave this
> > decision to the PWM user.   
> Do you thinks we should proceed with restoring the registers behind
> the re-apply as other drivers does at this moment?

Nope. IMO we'd better start patching PWM users to restore the states
rather than supporting suspend/resume in all PWM drivers.

Thierry, what's your opinion?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web