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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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

> On 10.04.2017 19:27, Boris Brezillon wrote:
> > On Mon, 10 Apr 2017 18:01:37 +0200
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> >> On Mon, 10 Apr 2017 17:10:11 +0200
> >> Thierry Reding <thierry.reding@gmail.com> wrote:
> >>  
> >>> On Mon, Apr 10, 2017 at 04:35:58PM +0200, 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].
> >>>>
> >>>> Thierry, maybe it's time to start thinking about a generic solution to
> >>>> save/restore PWM states.      
> >>>
> >>> Generally speaking I think applying the states are the right way to go.
> >>> Ideally the PWM core could simply resume all of the PWM channels that a
> >>> device exports and the ->apply() callback would be enough to restore
> >>> that. I'm not sure if that's going to work with current implementations,
> >>> though. Your pwm-atmel-hlcdc patch certainly indicates that we're not
> >>> quite there yet.
> >>>
> >>> On the other hand, I'm beginning to think that maybe PWMs are too low-
> >>> level for this kind of suspend/resume. For example if you use the PWM to
> >>> control a backlight brightness, restoring it via the driver core's
> >>> resume hook is potentially going to turn it back on at the wrong time. I
> >>> have a feeling that we might be better off just pushing this up to the
> >>> PWM users. A slight special case might be sysfs, for which no external
> >>> user driver exists. But we already have separate data structures to keep
> >>> track of sysfs-related context, so suspend/resume support could be added
> >>> there.    
> >>
> >> Yep, you're probably right, we should let the PWM user take care of
> >> re-applying the PWM state, because it's the only one having enough
> >> knowledge about what the PWM is really driving to take a wise decision.  
> > 
> > Note that we need drivers to implement both ->apply() and ->get_state()
> > for this approach to work correctly, and we also need some help from
> > the core to reset the PWM states at resume time, otherwise
> > pwm_apply_state() will just compare the old state to the new one, see
> > that they match and never call the ->apply() method.
> > 
> > Another solution would be to remove the memcmp here [1] and
> > unconditionally call ->apply().  
> There are drivers which checks, in ->apply() hooks, the current PWM state
> before applying the new state or take actions based on differences
> b/w current and new PWM states. Removing memcmp without resetting
> the PWM state would lead to wrong states in those drivers.

Indeed. So it just leaves the solution where we implement ->get_state().
Honestly, it shouldn't be too hard to do that in the atmel driver.

Note that for drivers that do not implement ->get_state(), the first
pwm_apply_state() after the system has resumed should be harmless,
because the current PWM should exactly match the one the PWM user is
re-applying.