[PATCH v4 0/3] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[PATCH v4 0/3] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

Hi All,

As requested here is a new version of this series adding a patch exporting
acpi_device_get_power() and using that.

This means that the PWM changes now rely on an ACPI patch.

I've 3 other LPSS PWM changes in flight which all have already been
acked or reviewed by Andy. One option would be to merge the entire
series + the 3 patches through Rafael's PM tree. If we want to go that
route I can send out a new series with all 6 patches together to Rafael.

Another option would be to merge the single line ACPI change through the
PWM tree.

Rafael, Thierry how do you want to proceed with this? And if you prefer
for the changes to go through the other tree, may we have your Acked-by
for the patch(es) which would normally go in through your own tree ?

Regards,

Hans

[PATCH v4 1/3] ACPI / PM: Export acpi_device_get_power() for use by modular build drivers

[Hans de Goede]

Export acpi_device_get_power() for use by modular build drivers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
-New patch in v4 of this patch-set
---
 drivers/acpi/device_pm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index a7c2673ffd36..824ae985ad93 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -126,6 +126,7 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
 
 	return 0;
 }
+EXPORT_SYMBOL(acpi_device_get_power);
 
 static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
 {
-- 
2.19.0

[PATCH v4 2/3] pwm: lpss: Move struct pwm_lpss_chip definition to the header file

[Hans de Goede]

Move struct pwm_lpss_chip definition from pwm-lpss.c to pwm-lpss.h,
so that the pci/platform drivers can access the info member
(struct pwm_lpss_boardinfo *).

This is a preparation patch for adding platform specific quirks, which
the drivers need access to, to pwm_lpss_boardinfo.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
-No changes in v4 of this patch-set

Changes in v3:
-There was no v3, but I accidentally put v3 in the Subject of the v2
 patches, so lets skip v3

Changes in v2:
-No changes in v2 of this patch-set
---
 drivers/pwm/pwm-lpss.c | 9 ---------
 drivers/pwm/pwm-lpss.h | 9 ++++++++-
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 4721a264bac2..e602835fd6de 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -32,15 +32,6 @@
 /* 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)
 {
 	return container_of(chip, struct pwm_lpss_chip, chip);
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 7a4238ad1fcb..8f029ed263af 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -16,7 +16,14 @@
 #include <linux/device.h>
 #include <linux/pwm.h>
 
-struct pwm_lpss_chip;
+#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];
+};
 
 struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
-- 
2.19.0

[PATCH v4 3/3] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

The _PS0 method for the integrated graphics on some Cherry Trail devices
(observed on a HP Pavilion X2 10-p0XX) turns on the PWM chip (puts it in
D0), causing an inconsistency between the state the pm-core thinks it is
in (left runtime suspended as it was before the suspend/resume) and the
state it actually is in.

Interestingly enough this is done on a device where the pwm controller is
not used for the backlight at all, since it uses an eDP panel. On devices
where the PWM is used this is not a problem since we will resume it
ourselves anyways.

This inconsistency causes us to never suspend the pwm controller again,
which causes the device to not be able to reach S0ix states when suspended.

This commit adds a resume-complete handler, which when we think the device
is still run-time suspended checks the actual power-state and if necessary
updates the rpm-core's internal state.

This fixes the Pavilion X2 10-p0XX not reaching S0ix states when suspended.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
-Use acpi_device_get_power() instead of manually calling _PSC

Changes in v3:
-There was no v3, but I accidentally put v3 in the Subject of the v2
 patches, so lets skip v3

Changes in v2:
-Do the pm_runtime_en/disable before/after checking the power-state
---
 drivers/pwm/pwm-lpss-platform.c | 25 ++++++++++++++++++++++---
 drivers/pwm/pwm-lpss.h          |  2 ++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 7304f36ee715..b6edf8af26cc 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -30,6 +30,7 @@ static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
 	.clk_rate = 19200000,
 	.npwm = 1,
 	.base_unit_bits = 16,
+	.check_power_on_resume = true,
 };
 
 /* Broxton */
@@ -74,9 +75,27 @@ 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 void pwm_lpss_complete(struct device *dev)
+{
+	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
+	int ret, state;
+
+	/* The PWM may be turned on by AML code, update our state to match */
+	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
+		pm_runtime_disable(dev);
+
+		ret = acpi_device_get_power(ACPI_COMPANION(dev), &state);
+		if (ret == 0 && state == ACPI_STATE_D0)
+			pm_runtime_set_active(dev);
+
+		pm_runtime_enable(dev);
+	}
+}
+
+static const struct dev_pm_ops pwm_lpss_platform_pm_ops = {
+	.complete = pwm_lpss_complete,
+	SET_SYSTEM_SLEEP_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 },
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 8f029ed263af..1a2575d25bea 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -30,6 +30,8 @@ struct pwm_lpss_boardinfo {
 	unsigned int npwm;
 	unsigned long base_unit_bits;
 	bool bypass;
+	/* Some devices have AML code messing with the state underneath us */
+	bool check_power_on_resume;
 };
 
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
-- 
2.19.0

Re: [PATCH v4 1/3] ACPI / PM: Export acpi_device_get_power() for use by modular build drivers

[Andy Shevchenko]

On Thu, Oct 11, 2018 at 06:14:42PM +0200, Hans de Goede wrote:
> Export acpi_device_get_power() for use by modular build drivers.
> 

non-GPL export is used to be consistent with setter counterpart, right?

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

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> -New patch in v4 of this patch-set
> ---
>  drivers/acpi/device_pm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index a7c2673ffd36..824ae985ad93 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -126,6 +126,7 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(acpi_device_get_power);
>  
>  static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
>  {
> -- 
> 2.19.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] pwm: lpss: Move struct pwm_lpss_chip definition to the header file

[Andy Shevchenko]

On Thu, Oct 11, 2018 at 06:14:43PM +0200, Hans de Goede wrote:
> Move struct pwm_lpss_chip definition from pwm-lpss.c to pwm-lpss.h,
> so that the pci/platform drivers can access the info member
> (struct pwm_lpss_boardinfo *).
> 
> This is a preparation patch for adding platform specific quirks, which
> the drivers need access to, to pwm_lpss_boardinfo.


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

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> -No changes in v4 of this patch-set
> 
> Changes in v3:
> -There was no v3, but I accidentally put v3 in the Subject of the v2
>  patches, so lets skip v3
> 
> Changes in v2:
> -No changes in v2 of this patch-set
> ---
>  drivers/pwm/pwm-lpss.c | 9 ---------
>  drivers/pwm/pwm-lpss.h | 9 ++++++++-
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 4721a264bac2..e602835fd6de 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -32,15 +32,6 @@
>  /* 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)
>  {
>  	return container_of(chip, struct pwm_lpss_chip, chip);
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 7a4238ad1fcb..8f029ed263af 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -16,7 +16,14 @@
>  #include <linux/device.h>
>  #include <linux/pwm.h>
>  
> -struct pwm_lpss_chip;
> +#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];
> +};
>  
>  struct pwm_lpss_boardinfo {
>  	unsigned long clk_rate;
> -- 
> 2.19.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Andy Shevchenko]

On Thu, Oct 11, 2018 at 06:14:44PM +0200, Hans de Goede wrote:
> The _PS0 method for the integrated graphics on some Cherry Trail devices
> (observed on a HP Pavilion X2 10-p0XX) turns on the PWM chip (puts it in
> D0), causing an inconsistency between the state the pm-core thinks it is
> in (left runtime suspended as it was before the suspend/resume) and the
> state it actually is in.
> 
> Interestingly enough this is done on a device where the pwm controller is
> not used for the backlight at all, since it uses an eDP panel. On devices
> where the PWM is used this is not a problem since we will resume it
> ourselves anyways.
> 
> This inconsistency causes us to never suspend the pwm controller again,
> which causes the device to not be able to reach S0ix states when suspended.
> 
> This commit adds a resume-complete handler, which when we think the device
> is still run-time suspended checks the actual power-state and if necessary
> updates the rpm-core's internal state.
> 
> This fixes the Pavilion X2 10-p0XX not reaching S0ix states when suspended.

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

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> -Use acpi_device_get_power() instead of manually calling _PSC
> 
> Changes in v3:
> -There was no v3, but I accidentally put v3 in the Subject of the v2
>  patches, so lets skip v3
> 
> Changes in v2:
> -Do the pm_runtime_en/disable before/after checking the power-state
> ---
>  drivers/pwm/pwm-lpss-platform.c | 25 ++++++++++++++++++++++---
>  drivers/pwm/pwm-lpss.h          |  2 ++
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
> index 7304f36ee715..b6edf8af26cc 100644
> --- a/drivers/pwm/pwm-lpss-platform.c
> +++ b/drivers/pwm/pwm-lpss-platform.c
> @@ -30,6 +30,7 @@ static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
>  	.clk_rate = 19200000,
>  	.npwm = 1,
>  	.base_unit_bits = 16,
> +	.check_power_on_resume = true,
>  };
>  
>  /* Broxton */
> @@ -74,9 +75,27 @@ 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 void pwm_lpss_complete(struct device *dev)
> +{
> +	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
> +	int ret, state;
> +
> +	/* The PWM may be turned on by AML code, update our state to match */
> +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
> +		pm_runtime_disable(dev);
> +
> +		ret = acpi_device_get_power(ACPI_COMPANION(dev), &state);
> +		if (ret == 0 && state == ACPI_STATE_D0)
> +			pm_runtime_set_active(dev);
> +
> +		pm_runtime_enable(dev);
> +	}
> +}
> +
> +static const struct dev_pm_ops pwm_lpss_platform_pm_ops = {
> +	.complete = pwm_lpss_complete,
> +	SET_SYSTEM_SLEEP_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 },
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 8f029ed263af..1a2575d25bea 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -30,6 +30,8 @@ struct pwm_lpss_boardinfo {
>  	unsigned int npwm;
>  	unsigned long base_unit_bits;
>  	bool bypass;
> +	/* Some devices have AML code messing with the state underneath us */
> +	bool check_power_on_resume;
>  };
>  
>  struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
> -- 
> 2.19.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/3] ACPI / PM: Export acpi_device_get_power() for use by modular build drivers

[Rafael J. Wysocki]

On Thursday, October 11, 2018 6:14:42 PM CEST Hans de Goede wrote:
> Export acpi_device_get_power() for use by modular build drivers.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> -New patch in v4 of this patch-set
> ---
>  drivers/acpi/device_pm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index a7c2673ffd36..824ae985ad93 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -126,6 +126,7 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(acpi_device_get_power);
>  
>  static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
>  {
> 

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Re: [PATCH v4 1/3] ACPI / PM: Export acpi_device_get_power() for use by modular build drivers

[Hans de Goede]

Hi,

On 11-10-18 22:37, Rafael J. Wysocki wrote:
> On Thursday, October 11, 2018 6:14:42 PM CEST Hans de Goede wrote:
>> Export acpi_device_get_power() for use by modular build drivers.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v4:
>> -New patch in v4 of this patch-set
>> ---
>>   drivers/acpi/device_pm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index a7c2673ffd36..824ae985ad93 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -126,6 +126,7 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(acpi_device_get_power);
>>   
>>   static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
>>   {
>>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Does this mean that you are ok with taking this patch
upstream through Thierry's PWM tree ?

Thierry, do you want me to resend the latest version of all my
outstanding PWM LPSS patches as a single series?

Regards,

Hans

Re: [PATCH v4 1/3] ACPI / PM: Export acpi_device_get_power() for use by modular build drivers

[Hans de Goede]

Hi,

On 11-10-18 19:14, Andy Shevchenko wrote:
> On Thu, Oct 11, 2018 at 06:14:42PM +0200, Hans de Goede wrote:
>> Export acpi_device_get_power() for use by modular build drivers.
>>
> 
> non-GPL export is used to be consistent with setter counterpart, right?

Right.

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

Thanks.

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v4:
>> -New patch in v4 of this patch-set
>> ---
>>   drivers/acpi/device_pm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> index a7c2673ffd36..824ae985ad93 100644
>> --- a/drivers/acpi/device_pm.c
>> +++ b/drivers/acpi/device_pm.c
>> @@ -126,6 +126,7 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(acpi_device_get_power);
>>   
>>   static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
>>   {
>> -- 
>> 2.19.0
>>
> 

Re: [PATCH v4 1/3] ACPI / PM: Export acpi_device_get_power() for use by modular build drivers

[Rafael J. Wysocki]

On Fri, Oct 12, 2018 at 11:29 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11-10-18 22:37, Rafael J. Wysocki wrote:
> > On Thursday, October 11, 2018 6:14:42 PM CEST Hans de Goede wrote:
> >> Export acpi_device_get_power() for use by modular build drivers.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Changes in v4:
> >> -New patch in v4 of this patch-set
> >> ---
> >>   drivers/acpi/device_pm.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> >> index a7c2673ffd36..824ae985ad93 100644
> >> --- a/drivers/acpi/device_pm.c
> >> +++ b/drivers/acpi/device_pm.c
> >> @@ -126,6 +126,7 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
> >>
> >>      return 0;
> >>   }
> >> +EXPORT_SYMBOL(acpi_device_get_power);
> >>
> >>   static int acpi_dev_pm_explicit_set(struct acpi_device *adev, int state)
> >>   {
> >>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Does this mean that you are ok with taking this patch
> upstream through Thierry's PWM tree ?

Yes, it does.

Thanks,
Rafael