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

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

[Claudiu Beznea]

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];
 
 	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);
+
+	return 0;
+}
+
+static int atmel_pwm_resume(struct device *dev)
+{
+	struct atmel_pwm_chip *atmel_pwm = dev_get_drvdata(dev);
+	struct pwm_device *pwm = atmel_pwm->chip.pwms;
+	int i, ret;
+	bool disable_clk = true;
+
+	ret = clk_enable(atmel_pwm->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	for (i = 0; i < atmel_pwm->chip.npwm; i++, pwm++) {
+		if (!pwm_is_enabled(pwm))
+			continue;
+
+		disable_clk = false;
+		atmel_pwm_ch_writel(atmel_pwm, i, PWM_CMR,
+				    atmel_pwm->ctx[i].cmr);
+		atmel_pwm_set_cprd_cdty(&atmel_pwm->chip, pwm,
+					atmel_pwm->ctx[i].cprd,
+					atmel_pwm->ctx[i].cdty);
+		mutex_lock(&atmel_pwm->isr_lock);
+		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+		atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
+		mutex_unlock(&atmel_pwm->isr_lock);
+		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
+	}
+
+	if (disable_clk)
+		clk_disable(atmel_pwm->clk);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(atmel_pwm_pm_ops,
+			 atmel_pwm_suspend, atmel_pwm_resume);
+
 static int atmel_pwm_probe(struct platform_device *pdev)
 {
 	const struct atmel_pwm_registers *regs;
@@ -406,6 +486,7 @@ static struct platform_driver atmel_pwm_driver = {
 	.driver = {
 		.name = "atmel-pwm",
 		.of_match_table = of_match_ptr(atmel_pwm_dt_ids),
+		.pm = &atmel_pwm_pm_ops,
 	},
 	.id_table = atmel_pwm_devtypes,
 	.probe = atmel_pwm_probe,
-- 
2.7.4

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

[Boris Brezillon]

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.

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

[1]http://patchwork.ozlabs.org/patch/734306/

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

[Thierry Reding]

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.

Any thoughts on that?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170410/8dd56f59/attachment-0001.sig>

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

[Boris Brezillon]

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.

This goes against my patch adding suspend/resume hooks to the
pwm-atmel-hlcdc driver, but we can easily drop the call to
atmel_hlcdc_pwm_apply() in ->resume() once we have patched the
pwm-backlight driver to take care of that.

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

[Boris Brezillon]

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

[1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L466

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

[m18063]

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.
 
> 
> [1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L466
> 

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

[Boris Brezillon]

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.

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

[m18063]

On 11.04.2017 11:50, Boris Brezillon wrote:
> 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.
I agree.
> 
> 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.
I agree.

> 

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

[m18063]

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

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

[Boris Brezillon]

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.

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

[m18063]

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

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

[Boris Brezillon]

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?

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

[Thierry Reding]

On Tue, Apr 11, 2017 at 11:53:11AM +0200, Boris Brezillon wrote:
> 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?

I just noticed this thread while cleaning up patchwork. I think I had
already mentioned in an earlier reply that in my opinion we should leave
PWM suspend/resume to users.

I'm totally fine if we add helpers to the PWM core to help with that
task. Maybe something like this would work:

	void pwm_suspend(struct pwm_device *pwm)
	{
		pwm_get_state(pwm, &pwm->suspend);
		pwm_disable(pwm);
	}

	void pwm_resume(struct pwm_device *pwm)
	{
		pwm_apply_state(pwm, &pwm->suspend);
	}

Though, quite frankly, this is so trivial that drivers could just do
that themselves. Also, the helpers above aren't flexible at all with
respect to any special sequences the PWM might need to go through on
suspend. I suspect that this doesn't matter at all in most cases but
given how trivial they are we might as well just make drivers do it.
Also we don't burden users that don't care about suspend/resume with
the extra suspend state in struct pwm_device.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171205/4db88999/attachment.sig>

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

[Claudiu Beznea]

On 05.12.2017 11:06, Thierry Reding wrote:
> On Tue, Apr 11, 2017 at 11:53:11AM +0200, Boris Brezillon wrote:
>> 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?
> 
> I just noticed this thread while cleaning up patchwork. I think I had
> already mentioned in an earlier reply that in my opinion we should leave
> PWM suspend/resume to users.
What about the case where PWM was requested via sysfs?
> 
> I'm totally fine if we add helpers to the PWM core to help with that
> task. Maybe something like this would work:
> 
> 	void pwm_suspend(struct pwm_device *pwm)
> 	{
> 		pwm_get_state(pwm, &pwm->suspend);
> 		pwm_disable(pwm);
> 	}
> 
> 	void pwm_resume(struct pwm_device *pwm)
> 	{
> 		pwm_apply_state(pwm, &pwm->suspend);
> 	}
> 
> Though, quite frankly, this is so trivial that drivers could just do
> that themselves. Also, the helpers above aren't flexible at all with
> respect to any special sequences the PWM might need to go through on
> suspend. I suspect that this doesn't matter at all in most cases but
> given how trivial they are we might as well just make drivers do it.
> Also we don't burden users that don't care about suspend/resume with
> the extra suspend state in struct pwm_device.
> 
> Thierry
>