[PATCH V2 1/2] platform/x86/intel/pmc: Add resume callback

[PATCH V2 1/2] platform/x86/intel/pmc: Add resume callback

[David E. Box]

Add a resume callback to perform platform specific functions during resume
from suspend.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2 - Change resume_fixup to just resume and use it a a replacement for
     the regular resume flow (now called pmc_core_resume_common) if it
     exist. Suggested by Ilpo.

 drivers/platform/x86/intel/pmc/core.c | 14 ++++++++++++--
 drivers/platform/x86/intel/pmc/core.h |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index da6e7206d38b..bb2f11251f73 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -1223,11 +1223,11 @@ static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
 	return false;
 }
 
-static __maybe_unused int pmc_core_resume(struct device *dev)
+int pmc_core_resume_common(struct pmc_dev *pmcdev)
 {
-	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
 	const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
 	int offset = pmcdev->map->lpm_status_offset;
+	struct device *dev = &pmcdev->pdev->dev;
 
 	/* Check if the syspend used S0ix */
 	if (pm_suspend_via_firmware())
@@ -1257,6 +1257,16 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
 	return 0;
 }
 
+static __maybe_unused int pmc_core_resume(struct device *dev)
+{
+	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
+
+	if (pmcdev->resume)
+		return pmcdev->resume(pmcdev);
+
+	return pmc_core_resume_common(pmcdev);
+}
+
 static const struct dev_pm_ops pmc_core_pm_ops = {
 	SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
 };
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 9ca9b9746719..7c95586e742b 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -327,6 +327,7 @@ struct pmc_reg_map {
  * @lpm_en_modes:	Array of enabled modes from lowest to highest priority
  * @lpm_req_regs:	List of substate requirements
  * @core_configure:	Function pointer to configure the platform
+ * @resume:		Function to perform platform specific resume
  *
  * pmc_dev contains info about power management controller device.
  */
@@ -345,6 +346,7 @@ struct pmc_dev {
 	int lpm_en_modes[LPM_MAX_NUM_MODES];
 	u32 *lpm_req_regs;
 	void (*core_configure)(struct pmc_dev *pmcdev);
+	int (*resume)(struct pmc_dev *pmcdev);
 };
 
 extern const struct pmc_bit_map msr_map[];
@@ -398,6 +400,7 @@ extern const struct pmc_reg_map mtl_reg_map;
 extern void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
 extern int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
 
+int pmc_core_resume_common(struct pmc_dev *pmcdev);
 void spt_core_init(struct pmc_dev *pmcdev);
 void cnp_core_init(struct pmc_dev *pmcdev);
 void icl_core_init(struct pmc_dev *pmcdev);

base-commit: 86f67fe2db439867f9476c9b78ea3ebd4a06a123
-- 
2.34.1

[PATCH V2 2/2] platform/x86/intel/pmc/mtl: Put devices in D3 during resume

[David E. Box]

An earlier commit placed some driverless devices in D3 during boot so that
they don't block package cstate entry on Meteor Lake. Also place these
devices in D3 after resume from suspend.

Fixes: 336ba968d3e3 ("platform/x86/intel/pmc/mtl: Put GNA/IPU/VPU devices in D3")
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2 - rename mtl_fixup to mtl_d3_fixup. Call it from new mtl_resume
     function, followed by the common resume. Suggested by Ilpo.

 drivers/platform/x86/intel/pmc/mtl.c | 29 ++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
index e8cc156412ce..2b00ad9da621 100644
--- a/drivers/platform/x86/intel/pmc/mtl.c
+++ b/drivers/platform/x86/intel/pmc/mtl.c
@@ -68,16 +68,29 @@ static void mtl_set_device_d3(unsigned int device)
 	}
 }
 
-void mtl_core_init(struct pmc_dev *pmcdev)
+/*
+ * Set power state of select devices that do not have drivers to D3
+ * so that they do not block Package C entry.
+ */
+static void mtl_d3_fixup(void)
 {
-	pmcdev->map = &mtl_reg_map;
-	pmcdev->core_configure = mtl_core_configure;
-
-	/*
-	 * Set power state of select devices that do not have drivers to D3
-	 * so that they do not block Package C entry.
-	 */
 	mtl_set_device_d3(MTL_GNA_PCI_DEV);
 	mtl_set_device_d3(MTL_IPU_PCI_DEV);
 	mtl_set_device_d3(MTL_VPU_PCI_DEV);
 }
+
+static int mtl_resume(struct pmc_dev *pmcdev)
+{
+	mtl_d3_fixup();
+	return pmc_core_resume_common(pmcdev);
+}
+
+void mtl_core_init(struct pmc_dev *pmcdev)
+{
+	pmcdev->map = &mtl_reg_map;
+	pmcdev->core_configure = mtl_core_configure;
+
+	mtl_d3_fixup();
+
+	pmcdev->resume = mtl_resume;
+}
-- 
2.34.1

Re: [PATCH V2 2/2] platform/x86/intel/pmc/mtl: Put devices in D3 during resume

[Ilpo Järvinen]

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

On Wed, 7 Jun 2023, David E. Box wrote:

> An earlier commit placed some driverless devices in D3 during boot so that
> they don't block package cstate entry on Meteor Lake. Also place these
> devices in D3 after resume from suspend.
> 
> Fixes: 336ba968d3e3 ("platform/x86/intel/pmc/mtl: Put GNA/IPU/VPU devices in D3")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2 - rename mtl_fixup to mtl_d3_fixup. Call it from new mtl_resume
>      function, followed by the common resume. Suggested by Ilpo.
> 
>  drivers/platform/x86/intel/pmc/mtl.c | 29 ++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> index e8cc156412ce..2b00ad9da621 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -68,16 +68,29 @@ static void mtl_set_device_d3(unsigned int device)
>  	}
>  }
>  
> -void mtl_core_init(struct pmc_dev *pmcdev)
> +/*
> + * Set power state of select devices that do not have drivers to D3
> + * so that they do not block Package C entry.
> + */
> +static void mtl_d3_fixup(void)
>  {
> -	pmcdev->map = &mtl_reg_map;
> -	pmcdev->core_configure = mtl_core_configure;
> -
> -	/*
> -	 * Set power state of select devices that do not have drivers to D3
> -	 * so that they do not block Package C entry.
> -	 */
>  	mtl_set_device_d3(MTL_GNA_PCI_DEV);
>  	mtl_set_device_d3(MTL_IPU_PCI_DEV);
>  	mtl_set_device_d3(MTL_VPU_PCI_DEV);
>  }
> +
> +static int mtl_resume(struct pmc_dev *pmcdev)
> +{
> +	mtl_d3_fixup();
> +	return pmc_core_resume_common(pmcdev);
> +}
> +
> +void mtl_core_init(struct pmc_dev *pmcdev)
> +{
> +	pmcdev->map = &mtl_reg_map;
> +	pmcdev->core_configure = mtl_core_configure;
> +
> +	mtl_d3_fixup();
> +
> +	pmcdev->resume = mtl_resume;
> +}

Thanks. Looks good now,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH V2 1/2] platform/x86/intel/pmc: Add resume callback

[Ilpo Järvinen]

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

On Wed, 7 Jun 2023, David E. Box wrote:

> Add a resume callback to perform platform specific functions during resume
> from suspend.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH V2 2/2] platform/x86/intel/pmc/mtl: Put devices in D3 during resume

[Hans de Goede]

Hi David,

On 6/8/23 01:38, David E. Box wrote:
> An earlier commit placed some driverless devices in D3 during boot so that
> they don't block package cstate entry on Meteor Lake. Also place these
> devices in D3 after resume from suspend.
> 
> Fixes: 336ba968d3e3 ("platform/x86/intel/pmc/mtl: Put GNA/IPU/VPU devices in D3")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Thank you for your patch.

There is one thing which has me worried here:

What about when real proper drivers show up for these blocks?

I know that at least some people will likely be using the out of tree IPU6 driver with the IPU block.

And having 2 different drivers poke at the hw state seems like a bad idea to me.

Maybe we can add a check if no driver is bound and only set the state to D3 if no driver is bound?

Regards,

Hans



> ---
> 
> V2 - rename mtl_fixup to mtl_d3_fixup. Call it from new mtl_resume
>      function, followed by the common resume. Suggested by Ilpo.
> 
>  drivers/platform/x86/intel/pmc/mtl.c | 29 ++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> index e8cc156412ce..2b00ad9da621 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -68,16 +68,29 @@ static void mtl_set_device_d3(unsigned int device)
>  	}
>  }
>  
> -void mtl_core_init(struct pmc_dev *pmcdev)
> +/*
> + * Set power state of select devices that do not have drivers to D3
> + * so that they do not block Package C entry.
> + */
> +static void mtl_d3_fixup(void)
>  {
> -	pmcdev->map = &mtl_reg_map;
> -	pmcdev->core_configure = mtl_core_configure;
> -
> -	/*
> -	 * Set power state of select devices that do not have drivers to D3
> -	 * so that they do not block Package C entry.
> -	 */
>  	mtl_set_device_d3(MTL_GNA_PCI_DEV);
>  	mtl_set_device_d3(MTL_IPU_PCI_DEV);
>  	mtl_set_device_d3(MTL_VPU_PCI_DEV);
>  }
> +
> +static int mtl_resume(struct pmc_dev *pmcdev)
> +{
> +	mtl_d3_fixup();
> +	return pmc_core_resume_common(pmcdev);
> +}
> +
> +void mtl_core_init(struct pmc_dev *pmcdev)
> +{
> +	pmcdev->map = &mtl_reg_map;
> +	pmcdev->core_configure = mtl_core_configure;
> +
> +	mtl_d3_fixup();
> +
> +	pmcdev->resume = mtl_resume;
> +}

Re: [PATCH V2 2/2] platform/x86/intel/pmc/mtl: Put devices in D3 during resume

[David E. Box]

Hi Hans,

On Mon, 2023-06-12 at 11:42 +0200, Hans de Goede wrote:
> Hi David,
> 
> On 6/8/23 01:38, David E. Box wrote:
> > An earlier commit placed some driverless devices in D3 during boot so that
> > they don't block package cstate entry on Meteor Lake. Also place these
> > devices in D3 after resume from suspend.
> > 
> > Fixes: 336ba968d3e3 ("platform/x86/intel/pmc/mtl: Put GNA/IPU/VPU devices in
> > D3")
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> 
> Thank you for your patch.
> 
> There is one thing which has me worried here:
> 
> What about when real proper drivers show up for these blocks?
> 
> I know that at least some people will likely be using the out of tree IPU6
> driver with the IPU block.
> 
> And having 2 different drivers poke at the hw state seems like a bad idea to
> me.
> 
> Maybe we can add a check if no driver is bound and only set the state to D3 if
> no driver is bound?

This check exists but is not shown in the patch. mtl_set_device_d3() gets the
device lock and checks to see if dev.driver is NULL before putting in D3. This
was checked with the GNA driver installed.

David

> 
> Regards,
> 
> Hans
> 
> 
> 
> > ---
> > 
> > V2 - rename mtl_fixup to mtl_d3_fixup. Call it from new mtl_resume
> >      function, followed by the common resume. Suggested by Ilpo.
> > 
> >  drivers/platform/x86/intel/pmc/mtl.c | 29 ++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/mtl.c
> > b/drivers/platform/x86/intel/pmc/mtl.c
> > index e8cc156412ce..2b00ad9da621 100644
> > --- a/drivers/platform/x86/intel/pmc/mtl.c
> > +++ b/drivers/platform/x86/intel/pmc/mtl.c
> > @@ -68,16 +68,29 @@ static void mtl_set_device_d3(unsigned int device)
> >         }
> >  }
> >  
> > -void mtl_core_init(struct pmc_dev *pmcdev)
> > +/*
> > + * Set power state of select devices that do not have drivers to D3
> > + * so that they do not block Package C entry.
> > + */
> > +static void mtl_d3_fixup(void)
> >  {
> > -       pmcdev->map = &mtl_reg_map;
> > -       pmcdev->core_configure = mtl_core_configure;
> > -
> > -       /*
> > -        * Set power state of select devices that do not have drivers to D3
> > -        * so that they do not block Package C entry.
> > -        */
> >         mtl_set_device_d3(MTL_GNA_PCI_DEV);
> >         mtl_set_device_d3(MTL_IPU_PCI_DEV);
> >         mtl_set_device_d3(MTL_VPU_PCI_DEV);
> >  }
> > +
> > +static int mtl_resume(struct pmc_dev *pmcdev)
> > +{
> > +       mtl_d3_fixup();
> > +       return pmc_core_resume_common(pmcdev);
> > +}
> > +
> > +void mtl_core_init(struct pmc_dev *pmcdev)
> > +{
> > +       pmcdev->map = &mtl_reg_map;
> > +       pmcdev->core_configure = mtl_core_configure;
> > +
> > +       mtl_d3_fixup();
> > +
> > +       pmcdev->resume = mtl_resume;
> > +}
> 

Re: [PATCH V2 2/2] platform/x86/intel/pmc/mtl: Put devices in D3 during resume

[Hans de Goede]

Hi,

On 6/12/23 20:02, David E. Box wrote:
> Hi Hans,
> 
> On Mon, 2023-06-12 at 11:42 +0200, Hans de Goede wrote:
>> Hi David,
>>
>> On 6/8/23 01:38, David E. Box wrote:
>>> An earlier commit placed some driverless devices in D3 during boot so that
>>> they don't block package cstate entry on Meteor Lake. Also place these
>>> devices in D3 after resume from suspend.
>>>
>>> Fixes: 336ba968d3e3 ("platform/x86/intel/pmc/mtl: Put GNA/IPU/VPU devices in
>>> D3")
>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>>
>> Thank you for your patch.
>>
>> There is one thing which has me worried here:
>>
>> What about when real proper drivers show up for these blocks?
>>
>> I know that at least some people will likely be using the out of tree IPU6
>> driver with the IPU block.
>>
>> And having 2 different drivers poke at the hw state seems like a bad idea to
>> me.
>>
>> Maybe we can add a check if no driver is bound and only set the state to D3 if
>> no driver is bound?
> 
> This check exists but is not shown in the patch. mtl_set_device_d3() gets the
> device lock and checks to see if dev.driver is NULL before putting in D3. This
> was checked with the GNA driver installed.

Ah, yes I remember this now from the original patch adding
mtl_set_device_d3(). Good.

Let me go and merge this right away then.

Regards,

Hans






>>> ---
>>>
>>> V2 - rename mtl_fixup to mtl_d3_fixup. Call it from new mtl_resume
>>>      function, followed by the common resume. Suggested by Ilpo.
>>>
>>>  drivers/platform/x86/intel/pmc/mtl.c | 29 ++++++++++++++++++++--------
>>>  1 file changed, 21 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/intel/pmc/mtl.c
>>> b/drivers/platform/x86/intel/pmc/mtl.c
>>> index e8cc156412ce..2b00ad9da621 100644
>>> --- a/drivers/platform/x86/intel/pmc/mtl.c
>>> +++ b/drivers/platform/x86/intel/pmc/mtl.c
>>> @@ -68,16 +68,29 @@ static void mtl_set_device_d3(unsigned int device)
>>>         }
>>>  }
>>>  
>>> -void mtl_core_init(struct pmc_dev *pmcdev)
>>> +/*
>>> + * Set power state of select devices that do not have drivers to D3
>>> + * so that they do not block Package C entry.
>>> + */
>>> +static void mtl_d3_fixup(void)
>>>  {
>>> -       pmcdev->map = &mtl_reg_map;
>>> -       pmcdev->core_configure = mtl_core_configure;
>>> -
>>> -       /*
>>> -        * Set power state of select devices that do not have drivers to D3
>>> -        * so that they do not block Package C entry.
>>> -        */
>>>         mtl_set_device_d3(MTL_GNA_PCI_DEV);
>>>         mtl_set_device_d3(MTL_IPU_PCI_DEV);
>>>         mtl_set_device_d3(MTL_VPU_PCI_DEV);
>>>  }
>>> +
>>> +static int mtl_resume(struct pmc_dev *pmcdev)
>>> +{
>>> +       mtl_d3_fixup();
>>> +       return pmc_core_resume_common(pmcdev);
>>> +}
>>> +
>>> +void mtl_core_init(struct pmc_dev *pmcdev)
>>> +{
>>> +       pmcdev->map = &mtl_reg_map;
>>> +       pmcdev->core_configure = mtl_core_configure;
>>> +
>>> +       mtl_d3_fixup();
>>> +
>>> +       pmcdev->resume = mtl_resume;
>>> +}
>>
> 

Re: [PATCH V2 1/2] platform/x86/intel/pmc: Add resume callback

[Hans de Goede]

Hi,

On 6/8/23 01:38, David E. Box wrote:
> Add a resume callback to perform platform specific functions during resume
> from suspend.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> V2 - Change resume_fixup to just resume and use it a a replacement for
>      the regular resume flow (now called pmc_core_resume_common) if it
>      exist. Suggested by Ilpo.


Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans





>  drivers/platform/x86/intel/pmc/core.c | 14 ++++++++++++--
>  drivers/platform/x86/intel/pmc/core.h |  3 +++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index da6e7206d38b..bb2f11251f73 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -1223,11 +1223,11 @@ static inline bool pmc_core_is_s0ix_failed(struct pmc_dev *pmcdev)
>  	return false;
>  }
>  
> -static __maybe_unused int pmc_core_resume(struct device *dev)
> +int pmc_core_resume_common(struct pmc_dev *pmcdev)
>  {
> -	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>  	const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
>  	int offset = pmcdev->map->lpm_status_offset;
> +	struct device *dev = &pmcdev->pdev->dev;
>  
>  	/* Check if the syspend used S0ix */
>  	if (pm_suspend_via_firmware())
> @@ -1257,6 +1257,16 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static __maybe_unused int pmc_core_resume(struct device *dev)
> +{
> +	struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> +	if (pmcdev->resume)
> +		return pmcdev->resume(pmcdev);
> +
> +	return pmc_core_resume_common(pmcdev);
> +}
> +
>  static const struct dev_pm_ops pmc_core_pm_ops = {
>  	SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume)
>  };
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index 9ca9b9746719..7c95586e742b 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -327,6 +327,7 @@ struct pmc_reg_map {
>   * @lpm_en_modes:	Array of enabled modes from lowest to highest priority
>   * @lpm_req_regs:	List of substate requirements
>   * @core_configure:	Function pointer to configure the platform
> + * @resume:		Function to perform platform specific resume
>   *
>   * pmc_dev contains info about power management controller device.
>   */
> @@ -345,6 +346,7 @@ struct pmc_dev {
>  	int lpm_en_modes[LPM_MAX_NUM_MODES];
>  	u32 *lpm_req_regs;
>  	void (*core_configure)(struct pmc_dev *pmcdev);
> +	int (*resume)(struct pmc_dev *pmcdev);
>  };
>  
>  extern const struct pmc_bit_map msr_map[];
> @@ -398,6 +400,7 @@ extern const struct pmc_reg_map mtl_reg_map;
>  extern void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
>  extern int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
>  
> +int pmc_core_resume_common(struct pmc_dev *pmcdev);
>  void spt_core_init(struct pmc_dev *pmcdev);
>  void cnp_core_init(struct pmc_dev *pmcdev);
>  void icl_core_init(struct pmc_dev *pmcdev);
> 
> base-commit: 86f67fe2db439867f9476c9b78ea3ebd4a06a123