diff mbox series

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

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

Commit Message

David E. Box June 2, 2023, 11:21 p.m. UTC 
  An earlier commit placed some driverless devices in D3 during boot so that
they don't block package cstate entry. 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>
---
 drivers/platform/x86/intel/pmc/mtl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Ilpo Järvinen June 5, 2023, 11:59 a.m. UTC | #1 
On Fri, 2 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. 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>
> ---
>  drivers/platform/x86/intel/pmc/mtl.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> index e8cc156412ce..d87c4597c6d4 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -68,11 +68,8 @@ static void mtl_set_device_d3(unsigned int device)
>  	}
>  }
>  
> -void mtl_core_init(struct pmc_dev *pmcdev)
> +static void mtl_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.
> @@ -81,3 +78,13 @@ void mtl_core_init(struct pmc_dev *pmcdev)
>  	mtl_set_device_d3(MTL_IPU_PCI_DEV);
>  	mtl_set_device_d3(MTL_VPU_PCI_DEV);

I'd prefer the function be called something related to d3 / power state / 
or some along those lines rather than something obscure such as 
mtl_fixup(). And you can move the comment to be a function comment now.

>  }
> +
> +void mtl_core_init(struct pmc_dev *pmcdev)
> +{
> +	pmcdev->map = &mtl_reg_map;
> +	pmcdev->core_configure = mtl_core_configure;
> +
> +	mtl_fixup();
> +
> +	pmcdev->resume_fixup = mtl_fixup;

I'm a bit on the edge here whether this is a good approach in long-term or 
if it would be better to just provide a way for the platform file to 
replace entire .resume() (for this task it's obviously enough but it 
feels a bit hacky to hook into one fixed place on resume path).

static __maybe_unused int pmc_core_resume(struct device *dev)
{
	if (pmcdev->resume)
		return pmcdev->resume();
	else
		return pmc_core_resume_common();
}

where pmc_core_resume_common() contains the current pmc_core_resume() 
contents.

mtl_resume() would just call the d3 func and the common resume functions.
David E. Box June 5, 2023, 2:52 p.m. UTC | #2 
On Mon, 2023-06-05 at 14:59 +0300, Ilpo Järvinen wrote:
> On Fri, 2 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. 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>
> > ---
> >  drivers/platform/x86/intel/pmc/mtl.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/mtl.c
> > b/drivers/platform/x86/intel/pmc/mtl.c
> > index e8cc156412ce..d87c4597c6d4 100644
> > --- a/drivers/platform/x86/intel/pmc/mtl.c
> > +++ b/drivers/platform/x86/intel/pmc/mtl.c
> > @@ -68,11 +68,8 @@ static void mtl_set_device_d3(unsigned int device)
> >         }
> >  }
> >  
> > -void mtl_core_init(struct pmc_dev *pmcdev)
> > +static void mtl_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.
> > @@ -81,3 +78,13 @@ void mtl_core_init(struct pmc_dev *pmcdev)
> >         mtl_set_device_d3(MTL_IPU_PCI_DEV);
> >         mtl_set_device_d3(MTL_VPU_PCI_DEV);
> 
> I'd prefer the function be called something related to d3 / power state / 
> or some along those lines rather than something obscure such as 
> mtl_fixup(). And you can move the comment to be a function comment now.

Okay.

> 
> >  }
> > +
> > +void mtl_core_init(struct pmc_dev *pmcdev)
> > +{
> > +       pmcdev->map = &mtl_reg_map;
> > +       pmcdev->core_configure = mtl_core_configure;
> > +
> > +       mtl_fixup();
> > +
> > +       pmcdev->resume_fixup = mtl_fixup;
> 
> I'm a bit on the edge here whether this is a good approach in long-term or 
> if it would be better to just provide a way for the platform file to 
> replace entire .resume() (for this task it's obviously enough but it 
> feels a bit hacky to hook into one fixed place on resume path).
> 
> static __maybe_unused int pmc_core_resume(struct device *dev)
> {
>         if (pmcdev->resume)
>                 return pmcdev->resume();
>         else
>                 return pmc_core_resume_common();
> }
> 
> where pmc_core_resume_common() contains the current pmc_core_resume() 
> contents.
> 
> mtl_resume() would just call the d3 func and the common resume functions.

Yeah, makes sense. There are several conditional tasks in the current resume
code, so trying to have a fixed place in there to call a platform specific
workaround may not be possible if we need to use this again for a different
reason.

David

> 
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
index e8cc156412ce..d87c4597c6d4 100644
--- a/drivers/platform/x86/intel/pmc/mtl.c
+++ b/drivers/platform/x86/intel/pmc/mtl.c
@@ -68,11 +68,8 @@  static void mtl_set_device_d3(unsigned int device)
 	}
 }
 
-void mtl_core_init(struct pmc_dev *pmcdev)
+static void mtl_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.
@@ -81,3 +78,13 @@  void mtl_core_init(struct pmc_dev *pmcdev)
 	mtl_set_device_d3(MTL_IPU_PCI_DEV);
 	mtl_set_device_d3(MTL_VPU_PCI_DEV);
 }
+
+void mtl_core_init(struct pmc_dev *pmcdev)
+{
+	pmcdev->map = &mtl_reg_map;
+	pmcdev->core_configure = mtl_core_configure;
+
+	mtl_fixup();
+
+	pmcdev->resume_fixup = mtl_fixup;
+}