| Message ID | 1491834020-3194-1-git-send-email-claudiu.beznea@microchip.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(+)