[PATCH v5] pwm: renesas-tpu: Add suspend/resume function

[PATCH v5] pwm: renesas-tpu: Add suspend/resume function

[Cao Van Dong]

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

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

Re: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3915 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function

[Cao Van Dong]

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
>

RE: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function

[Yoshihiro Shimoda]

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

RE: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function

[Yoshihiro Shimoda]

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