linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] pwm: renesas-tpu: Add suspend/resume function
@ 2019-05-27  2:22 Cao Van Dong
  2019-05-27 14:17 ` Thierry Reding
  0 siblings, 1 reply; 4+ messages in thread
From: Cao Van Dong @ 2019-05-27  2:22 UTC (permalink / raw)
  To: linux-renesas-soc, thierry.reding, horms+renesas, geert+renesas,
	broonie, linux-pwm
  Cc: yoshihiro.shimoda.uh, kuninori.morimoto.gx, h-inayoshi, na-hoan, cv-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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function
  2019-05-27  2:22 [PATCH v5] pwm: renesas-tpu: Add suspend/resume function Cao Van Dong
@ 2019-05-27 14:17 ` Thierry Reding
  2019-05-28  1:10   ` Cao Van Dong
  2019-05-28  8:09   ` Yoshihiro Shimoda
  0 siblings, 2 replies; 4+ messages in thread
From: Thierry Reding @ 2019-05-27 14:17 UTC (permalink / raw)
  To: Cao Van Dong
  Cc: linux-renesas-soc, horms+renesas, geert+renesas, broonie,
	linux-pwm, yoshihiro.shimoda.uh, kuninori.morimoto.gx,
	h-inayoshi, na-hoan

[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function
  2019-05-27 14:17 ` Thierry Reding
@ 2019-05-28  1:10   ` Cao Van Dong
  2019-05-28  8:09   ` Yoshihiro Shimoda
  1 sibling, 0 replies; 4+ messages in thread
From: Cao Van Dong @ 2019-05-28  1:10 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-renesas-soc, horms+renesas, geert+renesas, broonie,
	linux-pwm, yoshihiro.shimoda.uh, kuninori.morimoto.gx,
	h-inayoshi, na-hoan

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
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v5] pwm: renesas-tpu: Add suspend/resume function
  2019-05-27 14:17 ` Thierry Reding
  2019-05-28  1:10   ` Cao Van Dong
@ 2019-05-28  8:09   ` Yoshihiro Shimoda
  1 sibling, 0 replies; 4+ messages in thread
From: Yoshihiro Shimoda @ 2019-05-28  8:09 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-renesas-soc, horms+renesas, geert+renesas, broonie,
	linux-pwm, Kuninori Morimoto, hideo inayoshi, na-hoan,
	Cao Van Dong

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-05-28  8:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-27  2:22 [PATCH v5] pwm: renesas-tpu: Add suspend/resume function Cao Van Dong
2019-05-27 14:17 ` Thierry Reding
2019-05-28  1:10   ` Cao Van Dong
2019-05-28  8:09   ` Yoshihiro Shimoda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).