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

[PATCH v2] 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 v1 -> v2:
  - Repair the handling code to cover case of using multiple timers.
---
 drivers/pwm/pwm-renesas-tpu.c | 55 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 4a855a2..a6660cc 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -366,6 +366,60 @@ 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_restart_timer(struct pwm_device *pwm)
+{
+	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
+		return 0;
+
+	/* Restart timer */
+	tpu_pwm_disable(pwm->chip,pwm);
+	tpu_pwm_enable(pwm->chip,pwm);
+
+	return 0;
+}
+
+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 <= 3; i++) {
+		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
+			pwm = &chip->pwms[i];
+			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
+				return 0;
+		}
+	}
+
+	pm_runtime_put(dev);
+
+	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;
+
+	pm_runtime_get_sync(dev);
+
+	for (i = 0; i <= 3; i++) {
+		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
+			pwm = &chip->pwms[i];
+			tpu_pwm_restart_timer(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 +513,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 v2] pwm: renesas-tpu: Add suspend/resume function

[Kuninori Morimoto]

Hi

> +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 <= 3; i++) {
> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
> +			pwm = &chip->pwms[i];
> +			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> +				return 0;
> +		}
> +	}

why 3 ?

About code, how about this ?

		pwm = &chip->pwms[i];
		if (pwm_get_chip_data(pwm)) {
			...

Can we use chip->pwms at driver ? I'm not sure but pwm.h say

	struct pwm_chip {
		...
=>		/* only used internally by the PWM framework */
		struct list_head list;
		struct pwm_device *pwms;
	};


> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}

Do we need to call pm_runtime_xxx here ?

> +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;
> +
> +	pm_runtime_get_sync(dev);
> +
> +	for (i = 0; i <= 3; i++) {
> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
> +			pwm = &chip->pwms[i];
> +			tpu_pwm_restart_timer(pwm);
> +		}
> +	}

ditto

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

[Kuninori Morimoto]

Hi

> +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 <= 3; i++) {
> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
> +			pwm = &chip->pwms[i];
> +			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> +				return 0;
> +		}
> +	}

why 3 ?

About code, how about this ?

		pwm = &chip->pwms[i];
		if (pwm_get_chip_data(pwm)) {
			...

Can we use chip->pwms at driver ? I'm not sure but pwm.h say

	struct pwm_chip {
		...
=>		/* only used internally by the PWM framework */
		struct list_head list;
		struct pwm_device *pwms;
	};


> +
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}

Do we need to call pm_runtime_xxx here ?

> +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;
> +
> +	pm_runtime_get_sync(dev);
> +
> +	for (i = 0; i <= 3; i++) {
> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
> +			pwm = &chip->pwms[i];
> +			tpu_pwm_restart_timer(pwm);
> +		}
> +	}

ditto

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

[Cao Van Dong]

Dear Morimoto-san,

Thank for your feedback!

On 2019/05/23 11:02, Kuninori Morimoto wrote:
> Hi
>
>> +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 <= 3; i++) {
>> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
>> +			pwm = &chip->pwms[i];
>> +			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
>> +				return 0;
>> +		}
>> +	}
> why 3 ?
According to Hardware manual, 16-Bit Timer Pulse Unit (TPU)
supports four 16-bit timers for both R-car GEN2 and GEN3.
> About code, how about this ?
>
> 		pwm = &chip->pwms[i];
> 		if (pwm_get_chip_data(pwm)) {
> 			...
>
> Can we use chip->pwms at driver ? I'm not sure but pwm.h say
>
> 	struct pwm_chip {
> 		...
> =>		/* only used internally by the PWM framework */
> 		struct list_head list;
> 		struct pwm_device *pwms;
> 	};
As described in the header: "@pwms: array of PWM devices allocated by 
the framework"
Therefore, we can not use chip->pwms and must specify the device that 
will be used.
Otherwise the compilation will fail.
>> +
>> +	pm_runtime_put(dev);
>> +
>> +	return 0;
>> +}
> Do we need to call pm_runtime_xxx here ?

"pm_runtime_put(dev);" function is called for runtime idle operations.

>> +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;
>> +
>> +	pm_runtime_get_sync(dev);
>> +
>> +	for (i = 0; i <= 3; i++) {
>> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
>> +			pwm = &chip->pwms[i];
>> +			tpu_pwm_restart_timer(pwm);
>> +		}
>> +	}
> ditto

Thank you,

Dong

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

[Kuninori Morimoto]

Hi

> >> +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 <= 3; i++) {
> >> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
> >> +			pwm = &chip->pwms[i];
> >> +			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> >> +				return 0;
> >> +		}
> >> +	}
> > why 3 ?
> According to Hardware manual, 16-Bit Timer Pulse Unit (TPU)
> supports four 16-bit timers for both R-car GEN2 and GEN3.

Hmm...
You need to use chip->npwm or TPU_CHANNEL_MAX then ?

> >> +	pm_runtime_put(dev);
> >> +
> >> +	return 0;
> >> +}
> > Do we need to call pm_runtime_xxx here ?
> 
> "pm_runtime_put(dev);" function is called for runtime idle operations.

I know.
I'm asking do we need to call it here ?

Thank you for your help !!
Best regards
---
Kuninori Morimoto

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

[Kuninori Morimoto]

Hi

> >> +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 <= 3; i++) {
> >> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
> >> +			pwm = &chip->pwms[i];
> >> +			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> >> +				return 0;
> >> +		}
> >> +	}
> > why 3 ?
> According to Hardware manual, 16-Bit Timer Pulse Unit (TPU)
> supports four 16-bit timers for both R-car GEN2 and GEN3.

Hmm...
You need to use chip->npwm or TPU_CHANNEL_MAX then ?

> >> +	pm_runtime_put(dev);
> >> +
> >> +	return 0;
> >> +}
> > Do we need to call pm_runtime_xxx here ?
> 
> "pm_runtime_put(dev);" function is called for runtime idle operations.

I know.
I'm asking do we need to call it here ?

Thank you for your help !!
Best regards
---
Kuninori Morimoto

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

[Cao Van Dong]

Dear Morimoto-san,

Thank for your feedback!

On 2019/05/23 13:07, Kuninori Morimoto wrote:
> Hi
>
>>>> +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 <= 3; i++) {
>>>> +		if ((pwm_get_chip_data(&chip->pwms[i])) != NULL) {
>>>> +			pwm = &chip->pwms[i];
>>>> +			if (!test_bit(PWMF_REQUESTED, &pwm->flags))
>>>> +				return 0;
>>>> +		}
>>>> +	}
>>> why 3 ?
>> According to Hardware manual, 16-Bit Timer Pulse Unit (TPU)
>> supports four 16-bit timers for both R-car GEN2 and GEN3.
> Hmm...
> You need to use chip->npwm or TPU_CHANNEL_MAX then ?

Thank for your opinion!
I will resubmit v3 to change 3 to TPU_CHANNEL_MAX.

>>>> +	pm_runtime_put(dev);
>>>> +
>>>> +	return 0;
>>>> +}
>>> Do we need to call pm_runtime_xxx here ?
>> "pm_runtime_put(dev);" function is called for runtime idle operations.
> I know.
> I'm asking do we need to call it here ?

I think we should have it here better.

Thank you,
Dong

> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto