Re: [PATCH 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, 2018-04-13 at 14:52 +0200, Hans de Goede wrote:
> On some devices the contents of the ctrl register get lost over a
> suspend/resume and the PWM comes back up disabled after the resume.
> 
> This is seen on some Bay Trail devices with the PWM in ACPI enumerated
> mode, so it shows up as a platform device instead of a PCI device.
> 
> If we still think it is enabled and then try to change the duty-cycle
> after this, we end up with a "PWM_SW_UPDATE was not cleared" error and
> the PWM is stuck in that state from then on.
> 
> This commit adds suspend and resume pm callbacks to the pwm-lpss-
> platform
> code, which save/restore the ctrl register over a suspend/resume,
> fixing
> this.
> 
> Note that:
> 
> 1) There is no need to do this over a runtime suspend, since we
> only runtime suspend when disabled and then we properly set the enable
> bit and reprogram the timings when we re-enable the PWM.
> 
> 2) This may be happening on more systems then we realize, but has been
> covered up sofar by a bug in the acpi-lpss.c code which was
> save/restoring
> the regular device registers instead of the lpss private registers due
> to
> lpss_device_desc.prv_offset not being set. This is fixed by a later
> patch
> in this series.

One question below, the rest is fine by me

Reviwed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss-platform.c |  5 +++++

What about PCI variant?

>  drivers/pwm/pwm-lpss.c          | 30 ++++++++++++++++++++++++++++++
>  drivers/pwm/pwm-lpss.h          |  2 ++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-
> platform.c
> index 5d6ed1507d29..5561b9e190f8 100644
> --- a/drivers/pwm/pwm-lpss-platform.c
> +++ b/drivers/pwm/pwm-lpss-platform.c
> @@ -74,6 +74,10 @@ static int pwm_lpss_remove_platform(struct
> platform_device *pdev)
>  	return pwm_lpss_remove(lpwm);
>  }
>  
> +static SIMPLE_DEV_PM_OPS(pwm_lpss_platform_pm_ops,
> +			 pwm_lpss_suspend,
> +			 pwm_lpss_resume);
> +
>  static const struct acpi_device_id pwm_lpss_acpi_match[] = {
>  	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
>  	{ "80862288", (unsigned long)&pwm_lpss_bsw_info },
> @@ -86,6 +90,7 @@ static struct platform_driver
> pwm_lpss_driver_platform = {
>  	.driver = {
>  		.name = "pwm-lpss",
>  		.acpi_match_table = pwm_lpss_acpi_match,
> +		.pm = &pwm_lpss_platform_pm_ops,
>  	},
>  	.probe = pwm_lpss_probe_platform,
>  	.remove = pwm_lpss_remove_platform,
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 08d5cab0b8e8..49546a1d49ea 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -32,10 +32,13 @@
>  /* Size of each PWM register space if multiple */
>  #define PWM_SIZE			0x400
>  
> +#define MAX_PWMS			4
> +
>  struct pwm_lpss_chip {
>  	struct pwm_chip chip;
>  	void __iomem *regs;
>  	const struct pwm_lpss_boardinfo *info;
> +	u32 saved_ctrl[MAX_PWMS];
>  };
>  
>  static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
> @@ -225,6 +228,9 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device
> *dev, struct resource *r,
>  	unsigned long c;
>  	int ret;
>  
> +	if (WARN_ON(info->npwm > MAX_PWMS))
> +		return ERR_PTR(-ENODEV);
> +
>  	lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
>  	if (!lpwm)
>  		return ERR_PTR(-ENOMEM);
> @@ -264,6 +270,30 @@ int pwm_lpss_remove(struct pwm_lpss_chip *lpwm)
>  }
>  EXPORT_SYMBOL_GPL(pwm_lpss_remove);
>  
> +int pwm_lpss_suspend(struct device *dev)
> +{
> +	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < lpwm->info->npwm; i++)

> +		lpwm->saved_ctrl[i] = readl(lpwm->regs + i * PWM_SIZE
> + PWM);

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pwm_lpss_suspend);
> +
> +int pwm_lpss_resume(struct device *dev)
> +{
> +	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < lpwm->info->npwm; i++)
> +		writel(lpwm->saved_ctrl[i], lpwm->regs + i * PWM_SIZE
> + PWM);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pwm_lpss_resume);
> +
>  MODULE_DESCRIPTION("PWM driver for Intel LPSS");
>  MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 98306bb02cfe..7a4238ad1fcb 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -28,5 +28,7 @@ struct pwm_lpss_boardinfo {
>  struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct
> resource *r,
>  				     const struct pwm_lpss_boardinfo
> *info);
>  int pwm_lpss_remove(struct pwm_lpss_chip *lpwm);
> +int pwm_lpss_suspend(struct device *dev);
> +int pwm_lpss_resume(struct device *dev);
>  
>  #endif	/* __PWM_LPSS_H */

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy