| Message ID | 1558923757-9843-1-git-send-email-cv-dong@jinso.co.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v5] pwm: renesas-tpu: Add suspend/resume function | expand |
On Mon, May 27, 2019 at 11:22:37AM +0900, Cao Van Dong wrote: > This patch adds suspend/resume function support for Renesas the 16-Bit Timer > Pulse Unit (TPU) driver. This has been tested on the Salvator-XS board > with R-Car M3-N and H3 at renesas-drivers-2019-05-21-v5.2-rc1 tag. > I expect this to work on other SoCs. > > Test procedure: > - Enable TPU and pin control in DTS. > - Make sure switches { SW29-[1-2] are switched off or > SW31-[1-4] are switched off(only for Salvator-xs) }. > - Exercise userspace PWM control for pwm[2,3] > of /sys/class/pwm/pwmchip1/ . > - Inspect PWM signals on the input side of { CN29-[58,60] > or SW31-[1,2] (only for Salvator-xs) } > before and after suspend/resume using an oscilloscope. > > Signed-off-by: Cao Van Dong <cv-dong@jinso.co.jp> > Tested-by: Cao Van Dong <cv-dong@jinso.co.jp> > --- > Changes v4 -> v5: > - Remove test_bit(PWMF_REQUESTED, &pwm->flags) check. > --- > Changes v3 -> v4: > - Use pwm_is_enabled(pwm) to check channel instead of pwm_get_chip_data(&chip->pwms[i]). > - Move tpu_pwm_disable() to tpu_pwm_suspend(), tpu_pwm_enable() to tpu_pwm_resume(). > - Remove tpu_pwm_restart_timer() function and remove pm_runtime_get_sync() in tpu_pwm_resume(). > --- > Changes v2 -> v3: > - Changes '3' -> TPU_CHANNEL_MAX in loop. > - Remove pm_runtime_put() function in tpu_pwm_suspend() function. > --- > Changes v1 -> v2: > - Repair the handling code to cover case of using multiple timers. > --- > drivers/pwm/pwm-renesas-tpu.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) This has been discussed before, but this really shouldn't be done in the PWM driver. Consumers should really be reconfiguring the PWM upon resume as appropriate. This is the only way to ensure that everything is resumed in the proper order. Most, if not all, consumers already implement suspend/resume that way. sysfs is the only one that I'm aware of that doesn't. Since you've been using sysfs to test this, things are slightly more complicated (i.e. we don't have a consumer driver in the conventional way). However, you should be able to solve this by implementing dev_pm_ops for the pwm_class. Do you think you could give that a try? Thierry > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > index 4a855a2..86b7da4 100644 > --- a/drivers/pwm/pwm-renesas-tpu.c > +++ b/drivers/pwm/pwm-renesas-tpu.c > @@ -366,6 +366,41 @@ static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *_pwm) > tpu_pwm_timer_stop(pwm); > } > > +#ifdef CONFIG_PM_SLEEP > +static int tpu_pwm_suspend(struct device *dev) > +{ > + struct tpu_device *tpu = dev_get_drvdata(dev); > + struct pwm_chip *chip = &tpu->chip; > + struct pwm_device *pwm; > + int i; > + > + for (i = 0; i < TPU_CHANNEL_MAX; i++) { > + pwm = &chip->pwms[i]; > + if (pwm_is_enabled(pwm)) > + tpu_pwm_disable(pwm->chip, pwm); > + } > + > + return 0; > +} > + > +static int tpu_pwm_resume(struct device *dev) > +{ > + struct tpu_device *tpu = dev_get_drvdata(dev); > + struct pwm_chip *chip = &tpu->chip; > + struct pwm_device *pwm; > + int i; > + > + for (i = 0; i < TPU_CHANNEL_MAX; i++) { > + pwm = &chip->pwms[i]; > + if (pwm_is_enabled(pwm)) > + tpu_pwm_enable(pwm->chip, pwm); > + } > + > + return 0; > +} > +#endif /* CONFIG_PM_SLEEP */ > +static SIMPLE_DEV_PM_OPS(tpu_pwm_pm_ops, tpu_pwm_suspend, tpu_pwm_resume); > + > static const struct pwm_ops tpu_pwm_ops = { > .request = tpu_pwm_request, > .free = tpu_pwm_free, > @@ -459,6 +494,7 @@ static struct platform_driver tpu_driver = { > .remove = tpu_remove, > .driver = { > .name = "renesas-tpu-pwm", > + .pm = &tpu_pwm_pm_ops, > .of_match_table = of_match_ptr(tpu_of_table), > } > }; > -- > 2.7.4 >
Dear Thierry-san, Thank for your feedback! > This has been discussed before, but this really shouldn't be done in the > PWM driver. Consumers should really be reconfiguring the PWM upon resume > as appropriate. This is the only way to ensure that everything is > resumed in the proper order. > > Most, if not all, consumers already implement suspend/resume that way. > sysfs is the only one that I'm aware of that doesn't. > > Since you've been using sysfs to test this, things are slightly more > complicated (i.e. we don't have a consumer driver in the conventional > way). However, you should be able to solve this by implementing > dev_pm_ops for the pwm_class. > > Do you think you could give that a try? I understood the problem. Really this is difficult with me. Therefore, I will just stop here. Thank you, Dong > Thierry >
Hi Thierry, > From: Thierry Reding, Sent: Monday, May 27, 2019 11:18 PM <snip> > This has been discussed before, but this really shouldn't be done in the > PWM driver. Consumers should really be reconfiguring the PWM upon resume > as appropriate. This is the only way to ensure that everything is > resumed in the proper order. > > Most, if not all, consumers already implement suspend/resume that way. > sysfs is the only one that I'm aware of that doesn't. > > Since you've been using sysfs to test this, things are slightly more > complicated (i.e. we don't have a consumer driver in the conventional > way). However, you should be able to solve this by implementing > dev_pm_ops for the pwm_class. Thank you for your coment! I'm interesting about implementing dev_pm_ops for the pwm_class. This is because you talked related things on other pwm driver (pwm-rcar) on the following email thread. https://marc.info/?l=linux-renesas-soc&m=155169831832176&w=2 So, I'll try to implement it and tested on the pwm-rcar driver. Best regards, Yoshihiro Shimoda
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 4a855a2..86b7da4 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -366,6 +366,41 @@ static void tpu_pwm_disable(struct pwm_chip *chip, struct pwm_device *_pwm) tpu_pwm_timer_stop(pwm); } +#ifdef CONFIG_PM_SLEEP +static int tpu_pwm_suspend(struct device *dev) +{ + struct tpu_device *tpu = dev_get_drvdata(dev); + struct pwm_chip *chip = &tpu->chip; + struct pwm_device *pwm; + int i; + + for (i = 0; i < TPU_CHANNEL_MAX; i++) { + pwm = &chip->pwms[i]; + if (pwm_is_enabled(pwm)) + tpu_pwm_disable(pwm->chip, pwm); + } + + return 0; +} + +static int tpu_pwm_resume(struct device *dev) +{ + struct tpu_device *tpu = dev_get_drvdata(dev); + struct pwm_chip *chip = &tpu->chip; + struct pwm_device *pwm; + int i; + + for (i = 0; i < TPU_CHANNEL_MAX; i++) { + pwm = &chip->pwms[i]; + if (pwm_is_enabled(pwm)) + tpu_pwm_enable(pwm->chip, pwm); + } + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ +static SIMPLE_DEV_PM_OPS(tpu_pwm_pm_ops, tpu_pwm_suspend, tpu_pwm_resume); + static const struct pwm_ops tpu_pwm_ops = { .request = tpu_pwm_request, .free = tpu_pwm_free, @@ -459,6 +494,7 @@ static struct platform_driver tpu_driver = { .remove = tpu_remove, .driver = { .name = "renesas-tpu-pwm", + .pm = &tpu_pwm_pm_ops, .of_match_table = of_match_ptr(tpu_of_table), } };
This patch adds suspend/resume function support for Renesas the 16-Bit Timer Pulse Unit (TPU) driver. This has been tested on the Salvator-XS board with R-Car M3-N and H3 at renesas-drivers-2019-05-21-v5.2-rc1 tag. I expect this to work on other SoCs. Test procedure: - Enable TPU and pin control in DTS. - Make sure switches { SW29-[1-2] are switched off or SW31-[1-4] are switched off(only for Salvator-xs) }. - Exercise userspace PWM control for pwm[2,3] of /sys/class/pwm/pwmchip1/ . - Inspect PWM signals on the input side of { CN29-[58,60] or SW31-[1,2] (only for Salvator-xs) } before and after suspend/resume using an oscilloscope. Signed-off-by: Cao Van Dong <cv-dong@jinso.co.jp> Tested-by: Cao Van Dong <cv-dong@jinso.co.jp> --- Changes v4 -> v5: - Remove test_bit(PWMF_REQUESTED, &pwm->flags) check. --- Changes v3 -> v4: - Use pwm_is_enabled(pwm) to check channel instead of pwm_get_chip_data(&chip->pwms[i]). - Move tpu_pwm_disable() to tpu_pwm_suspend(), tpu_pwm_enable() to tpu_pwm_resume(). - Remove tpu_pwm_restart_timer() function and remove pm_runtime_get_sync() in tpu_pwm_resume(). --- Changes v2 -> v3: - Changes '3' -> TPU_CHANNEL_MAX in loop. - Remove pm_runtime_put() function in tpu_pwm_suspend() function. --- Changes v1 -> v2: - Repair the handling code to cover case of using multiple timers. --- drivers/pwm/pwm-renesas-tpu.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)