diff mbox series

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

Message ID 20230607233849.239047-2-david.e.box@linux.intel.com
State New
Headers
Series [V2,1/2] platform/x86/intel/pmc: Add resume callback |

Commit Message

David E. Box June 7, 2023, 11:38 p.m. UTC 
  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(-)

Comments

Ilpo Järvinen June 8, 2023, 2:32 p.m. UTC | #1 
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>
Hans de Goede June 12, 2023, 9:42 a.m. UTC | #2 
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;
> +}
David E. Box June 12, 2023, 6:02 p.m. UTC | #3 
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;
> > +}
>
Hans de Goede June 13, 2023, 10:29 a.m. UTC | #4 
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;
>>> +}
>>
>
diff mbox series

Patch

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;
+}