All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
@ 2018-04-26 12:10 Hans de Goede
  2018-04-26 12:10 ` [PATCH v2 2/2] ACPI / LPSS: Add missing prv_offset setting for byt/cht PWM devices Hans de Goede
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Hans de Goede @ 2018-04-26 12:10 UTC (permalink / raw)
  To: Thierry Reding, Andy Shevchenko, Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, linux-acpi, stable

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.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add Cc: stable@vger.kernel.org to make sure this goes into stable
 together with "ACPI / LPSS: Add missing prv_offset setting for byt/cht
 PWM devices" which depends on this
---
 drivers/pwm/pwm-lpss-platform.c |  5 +++++
 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 8db0d40ccacd..4721a264bac2 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)
@@ -177,6 +180,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);
@@ -212,6 +218,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 */
-- 
2.17.0

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

* [PATCH v2 2/2] ACPI / LPSS: Add missing prv_offset setting for byt/cht PWM devices
  2018-04-26 12:10 [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Hans de Goede
@ 2018-04-26 12:10 ` Hans de Goede
  2018-05-10 15:25 ` [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Rafael J. Wysocki
  2018-06-06  8:03 ` Thierry Reding
  2 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2018-04-26 12:10 UTC (permalink / raw)
  To: Thierry Reding, Andy Shevchenko, Rafael J . Wysocki, Len Brown
  Cc: Hans de Goede, linux-pwm, linux-acpi, stable

The LPSS PWM device on on Bay Trail and Cherry Trail devices has a set
of private registers at offset 0x800, the current lpss_device_desc for
them already sets the LPSS_SAVE_CTX flag to have these saved/restored
over device-suspend, but the current lpss_device_desc was not setting
the prv_offset field, leading to the regular device registers getting
saved/restored instead.

This is causing the PWM controller to no longer work, resulting in a black
screen,  after a suspend/resume on systems where the firmware clears the
APB clock and reset bits at offset 0x804.

This commit fixes this by properly setting prv_offset to 0x800 for
the PWM devices.

Cc: stable@vger.kernel.org
Fixes: e1c748179754 ("ACPI / LPSS: Add Intel BayTrail ACPI mode PWM")
Fixes: 1bfbd8eb8a7f ("ACPI / LPSS: Add ACPI IDs for Intel Braswell")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add Fixes and Cc:stable tags
---
 drivers/acpi/acpi_lpss.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 2bcffec8dbf0..c4ba9164e582 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -229,11 +229,13 @@ static const struct lpss_device_desc lpt_sdio_dev_desc = {
 
 static const struct lpss_device_desc byt_pwm_dev_desc = {
 	.flags = LPSS_SAVE_CTX,
+	.prv_offset = 0x800,
 	.setup = byt_pwm_setup,
 };
 
 static const struct lpss_device_desc bsw_pwm_dev_desc = {
 	.flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY,
+	.prv_offset = 0x800,
 	.setup = bsw_pwm_setup,
 };
 
-- 
2.17.0

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-04-26 12:10 [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Hans de Goede
  2018-04-26 12:10 ` [PATCH v2 2/2] ACPI / LPSS: Add missing prv_offset setting for byt/cht PWM devices Hans de Goede
@ 2018-05-10 15:25 ` Rafael J. Wysocki
  2018-05-14 11:10   ` Andy Shevchenko
  2018-05-14 11:50   ` Thierry Reding
  2018-06-06  8:03 ` Thierry Reding
  2 siblings, 2 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2018-05-10 15:25 UTC (permalink / raw)
  To: Hans de Goede, Thierry Reding, Andy Shevchenko
  Cc: Len Brown, linux-pwm, linux-acpi, stable

On Thursday, April 26, 2018 2:10:23 PM CEST 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Andy, Thierry, any comments or concerns regarding this series?

> ---
> Changes in v2:
> -Add Cc: stable@vger.kernel.org to make sure this goes into stable
>  together with "ACPI / LPSS: Add missing prv_offset setting for byt/cht
>  PWM devices" which depends on this
> ---
>  drivers/pwm/pwm-lpss-platform.c |  5 +++++
>  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 8db0d40ccacd..4721a264bac2 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)
> @@ -177,6 +180,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);
> @@ -212,6 +218,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 */
> 

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-05-10 15:25 ` [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Rafael J. Wysocki
@ 2018-05-14 11:10   ` Andy Shevchenko
  2018-05-14 11:50   ` Thierry Reding
  1 sibling, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2018-05-14 11:10 UTC (permalink / raw)
  To: Rafael J. Wysocki, Hans de Goede, Thierry Reding
  Cc: Len Brown, linux-pwm, linux-acpi, stable

On Thu, 2018-05-10 at 17:25 +0200, Rafael J. Wysocki wrote:
> On Thursday, April 26, 2018 2:10:23 PM CEST 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.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Andy, Thierry, any comments or concerns regarding this series?

Fine by me:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-05-10 15:25 ` [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Rafael J. Wysocki
  2018-05-14 11:10   ` Andy Shevchenko
@ 2018-05-14 11:50   ` Thierry Reding
  2018-05-14 14:08     ` Hans de Goede
  2018-05-14 20:36     ` Rafael J. Wysocki
  1 sibling, 2 replies; 9+ messages in thread
From: Thierry Reding @ 2018-05-14 11:50 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Hans de Goede, Andy Shevchenko, Len Brown, linux-pwm, linux-acpi, stable

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

On Thu, May 10, 2018 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
> On Thursday, April 26, 2018 2:10:23 PM CEST 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.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Andy, Thierry, any comments or concerns regarding this series?

Hans said in the cover letter of the first version of this series that
he preferred to merge both patches through the PWM tree because of the
dependency. So I'm waiting for an Acked-by from you on the ACPI bits.

Thierry

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

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-05-14 11:50   ` Thierry Reding
@ 2018-05-14 14:08     ` Hans de Goede
  2018-05-14 20:36     ` Rafael J. Wysocki
  1 sibling, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2018-05-14 14:08 UTC (permalink / raw)
  To: Thierry Reding, Rafael J. Wysocki
  Cc: Andy Shevchenko, Len Brown, linux-pwm, linux-acpi, stable

Hi,

On 05/14/2018 12:50 PM, Thierry Reding wrote:
> On Thu, May 10, 2018 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
>> On Thursday, April 26, 2018 2:10:23 PM CEST 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.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Andy, Thierry, any comments or concerns regarding this series?
> 
> Hans said in the cover letter of the first version of this series that
> he preferred to merge both patches through the PWM tree because of the
> dependency. So I'm waiting for an Acked-by from you on the ACPI bits.

The other way around (both through ACPI) would work too, but yes it
would be good to keep them together,

Regards,

Hans

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-05-14 11:50   ` Thierry Reding
  2018-05-14 14:08     ` Hans de Goede
@ 2018-05-14 20:36     ` Rafael J. Wysocki
  2018-06-01 13:54       ` Hans de Goede
  1 sibling, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2018-05-14 20:36 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rafael J. Wysocki, Hans de Goede, Andy Shevchenko, Len Brown,
	Linux PWM List, ACPI Devel Maling List, Stable

On Mon, May 14, 2018 at 1:50 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Thu, May 10, 2018 at 05:25:10PM +0200, Rafael J. Wysocki wrote:
>> On Thursday, April 26, 2018 2:10:23 PM CEST 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.
>> >
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Andy, Thierry, any comments or concerns regarding this series?
>
> Hans said in the cover letter of the first version of this series that
> he preferred to merge both patches through the PWM tree because of the
> dependency. So I'm waiting for an Acked-by from you on the ACPI bits.

OK, please feel free to add it to the patches, then.

Thanks,
Rafael

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-05-14 20:36     ` Rafael J. Wysocki
@ 2018-06-01 13:54       ` Hans de Goede
  0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2018-06-01 13:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Thierry Reding
  Cc: Rafael J. Wysocki, Andy Shevchenko, Len Brown, Linux PWM List,
	ACPI Devel Maling List, Stable

Hi Thierry,

Since Rafael has indicated that you can take both
patches upstream with his ack, through the pwm tree:

On 14-05-18 22:36, Rafael J. Wysocki wrote:
> On Mon, May 14, 2018 at 1:50 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:

<snip>

>> Hans said in the cover letter of the first version of this series that
>> he preferred to merge both patches through the PWM tree because of the
>> dependency. So I'm waiting for an Acked-by from you on the ACPI bits.
> 
> OK, please feel free to add it to the patches, then. >
> Thanks,
> Rafael

So can we get these 2 bugfix patches merged now please ?

Regards,

Hans

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

* Re: [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume
  2018-04-26 12:10 [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Hans de Goede
  2018-04-26 12:10 ` [PATCH v2 2/2] ACPI / LPSS: Add missing prv_offset setting for byt/cht PWM devices Hans de Goede
  2018-05-10 15:25 ` [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Rafael J. Wysocki
@ 2018-06-06  8:03 ` Thierry Reding
  2 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2018-06-06  8:03 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Andy Shevchenko, Rafael J . Wysocki, Len Brown, linux-pwm,
	linux-acpi, stable

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

On Thu, Apr 26, 2018 at 02:10:23PM +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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add Cc: stable@vger.kernel.org to make sure this goes into stable
>  together with "ACPI / LPSS: Add missing prv_offset setting for byt/cht
>  PWM devices" which depends on this
> ---
>  drivers/pwm/pwm-lpss-platform.c |  5 +++++
>  drivers/pwm/pwm-lpss.c          | 30 ++++++++++++++++++++++++++++++
>  drivers/pwm/pwm-lpss.h          |  2 ++
>  3 files changed, 37 insertions(+)

Applied, thanks.

Thierry

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

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

end of thread, other threads:[~2018-06-06  8:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-26 12:10 [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Hans de Goede
2018-04-26 12:10 ` [PATCH v2 2/2] ACPI / LPSS: Add missing prv_offset setting for byt/cht PWM devices Hans de Goede
2018-05-10 15:25 ` [PATCH v2 1/2] pwm: lpss: platform: Save/restore the ctrl register over a suspend/resume Rafael J. Wysocki
2018-05-14 11:10   ` Andy Shevchenko
2018-05-14 11:50   ` Thierry Reding
2018-05-14 14:08     ` Hans de Goede
2018-05-14 20:36     ` Rafael J. Wysocki
2018-06-01 13:54       ` Hans de Goede
2018-06-06  8:03 ` Thierry Reding

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.