[v1] platform/x86/intel/pmc Fix Arrow Lake U/H support to intel_pmc_core driver

[PATCH] platform/x86/intel/pmc Fix Arrow Lake U/H support to intel_pmc_core driver

Posted by Todd Brandt 7 months, 1 week ago

The ARL requires that the GMA and NPU devices both be in D3Hot in order
for PC10 and S0iX to be achieved in S2idle. The original ARL-H/U addition
to the intel_pmc_core driver attempted to do this by switching them to D3
in the init and resume calls of the intel_pmc_core driver.

The problem is the ARL-H/U have a different NPU device and thus are not
being properly set and thus S0iX does not work properly in ARL-H/U. This
patch creates a new ARL-H specific device id that is correct and also
adds the D3 fixup to the suspend callback. This way if the PCI devies
drop from D3 to D0 after resume they can be corrected for the next
suspend. Thus there is no dropout in S0iX.

Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
---
 drivers/platform/x86/intel/pmc/arl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
index 320993bd6d31..5053e3dd8f5e 100644
--- a/drivers/platform/x86/intel/pmc/arl.c
+++ b/drivers/platform/x86/intel/pmc/arl.c
@@ -681,6 +681,7 @@ static struct pmc_info arl_pmc_info_list[] = {
 
 #define ARL_NPU_PCI_DEV			0xad1d
 #define ARL_GNA_PCI_DEV			0xae4c
+#define ARL_H_NPU_PCI_DEV		0x7d1d
 #define ARL_H_GNA_PCI_DEV		0x774c
 /*
  * Set power state of select devices that do not have drivers to D3
@@ -694,7 +695,7 @@ static void arl_d3_fixup(void)
 
 static void arl_h_d3_fixup(void)
 {
-	pmc_core_set_device_d3(ARL_NPU_PCI_DEV);
+	pmc_core_set_device_d3(ARL_H_NPU_PCI_DEV);
 	pmc_core_set_device_d3(ARL_H_GNA_PCI_DEV);
 }
 
@@ -705,6 +706,13 @@ static int arl_resume(struct pmc_dev *pmcdev)
 	return cnl_resume(pmcdev);
 }
 
+static void arl_h_suspend(struct pmc_dev *pmcdev)
+{
+	arl_h_d3_fixup();
+
+	cnl_suspend(pmcdev);
+}
+
 static int arl_h_resume(struct pmc_dev *pmcdev)
 {
 	arl_h_d3_fixup();
@@ -739,7 +747,7 @@ struct pmc_dev_info arl_h_pmc_dev = {
 	.dmu_guid = ARL_PMT_DMU_GUID,
 	.regmap_list = arl_pmc_info_list,
 	.map = &mtl_socm_reg_map,
-	.suspend = cnl_suspend,
+	.suspend = arl_h_suspend,
 	.resume = arl_h_resume,
 	.init = arl_h_core_init,
 };
-- 
2.25.1

Re: [PATCH] platform/x86/intel/pmc Fix Arrow Lake U/H support to intel_pmc_core driver

Posted by Ilpo Järvinen 7 months, 1 week ago

On Thu, 15 May 2025, Todd Brandt wrote:

> The ARL requires that the GMA and NPU devices both be in D3Hot in order
> for PC10 and S0iX to be achieved in S2idle. The original ARL-H/U addition
> to the intel_pmc_core driver attempted to do this by switching them to D3
> in the init and resume calls of the intel_pmc_core driver.
> 
> The problem is the ARL-H/U have a different NPU device and thus are not
> being properly set and thus S0iX does not work properly in ARL-H/U. This
> patch creates a new ARL-H specific device id that is correct and also
> adds the D3 fixup to the suspend callback. This way if the PCI devies
> drop from D3 to D0 after resume they can be corrected for the next
> suspend. Thus there is no dropout in S0iX.
> 

Shouldn't this have a Fixes tag?

> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> ---
>  drivers/platform/x86/intel/pmc/arl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/arl.c b/drivers/platform/x86/intel/pmc/arl.c
> index 320993bd6d31..5053e3dd8f5e 100644
> --- a/drivers/platform/x86/intel/pmc/arl.c
> +++ b/drivers/platform/x86/intel/pmc/arl.c
> @@ -681,6 +681,7 @@ static struct pmc_info arl_pmc_info_list[] = {
>  
>  #define ARL_NPU_PCI_DEV			0xad1d
>  #define ARL_GNA_PCI_DEV			0xae4c
> +#define ARL_H_NPU_PCI_DEV		0x7d1d
>  #define ARL_H_GNA_PCI_DEV		0x774c
>  /*
>   * Set power state of select devices that do not have drivers to D3
> @@ -694,7 +695,7 @@ static void arl_d3_fixup(void)
>  
>  static void arl_h_d3_fixup(void)
>  {
> -	pmc_core_set_device_d3(ARL_NPU_PCI_DEV);
> +	pmc_core_set_device_d3(ARL_H_NPU_PCI_DEV);
>  	pmc_core_set_device_d3(ARL_H_GNA_PCI_DEV);
>  }
>  
> @@ -705,6 +706,13 @@ static int arl_resume(struct pmc_dev *pmcdev)
>  	return cnl_resume(pmcdev);
>  }
>  
> +static void arl_h_suspend(struct pmc_dev *pmcdev)
> +{
> +	arl_h_d3_fixup();
> +
> +	cnl_suspend(pmcdev);
> +}
> +
>  static int arl_h_resume(struct pmc_dev *pmcdev)
>  {
>  	arl_h_d3_fixup();
> @@ -739,7 +747,7 @@ struct pmc_dev_info arl_h_pmc_dev = {
>  	.dmu_guid = ARL_PMT_DMU_GUID,
>  	.regmap_list = arl_pmc_info_list,
>  	.map = &mtl_socm_reg_map,
> -	.suspend = cnl_suspend,
> +	.suspend = arl_h_suspend,
>  	.resume = arl_h_resume,
>  	.init = arl_h_core_init,
>  };
> 

-- 
 i.

RE: [PATCH] platform/x86/intel/pmc Fix Arrow Lake U/H support to intel_pmc_core driver

Posted by Brandt, Todd E 7 months, 1 week ago

Let me resend with a fixes tag (sorry about that), however this might be too late to be included in the upstream 6.15.0-rc7 kernel. So maybe we can slate this for 6.16.0-c1. I also want to get Xi's feedback and she's out this week.

-----Original Message-----
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> 
Sent: Friday, May 16, 2025 4:41 AM
To: Brandt, Todd E <todd.e.brandt@intel.com>
Cc: platform-driver-x86@vger.kernel.org; xi.pardee@linux.intel.com; LKML <linux-kernel@vger.kernel.org>; todd.e.brandt@linux.intel.com
Subject: Re: [PATCH] platform/x86/intel/pmc Fix Arrow Lake U/H support to intel_pmc_core driver

On Thu, 15 May 2025, Todd Brandt wrote:

> The ARL requires that the GMA and NPU devices both be in D3Hot in 
> order for PC10 and S0iX to be achieved in S2idle. The original ARL-H/U 
> addition to the intel_pmc_core driver attempted to do this by 
> switching them to D3 in the init and resume calls of the intel_pmc_core driver.
> 
> The problem is the ARL-H/U have a different NPU device and thus are 
> not being properly set and thus S0iX does not work properly in 
> ARL-H/U. This patch creates a new ARL-H specific device id that is 
> correct and also adds the D3 fixup to the suspend callback. This way 
> if the PCI devies drop from D3 to D0 after resume they can be 
> corrected for the next suspend. Thus there is no dropout in S0iX.
> 

Shouldn't this have a Fixes tag?

> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> ---
>  drivers/platform/x86/intel/pmc/arl.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/arl.c 
> b/drivers/platform/x86/intel/pmc/arl.c
> index 320993bd6d31..5053e3dd8f5e 100644
> --- a/drivers/platform/x86/intel/pmc/arl.c
> +++ b/drivers/platform/x86/intel/pmc/arl.c
> @@ -681,6 +681,7 @@ static struct pmc_info arl_pmc_info_list[] = {
>  
>  #define ARL_NPU_PCI_DEV			0xad1d
>  #define ARL_GNA_PCI_DEV			0xae4c
> +#define ARL_H_NPU_PCI_DEV		0x7d1d
>  #define ARL_H_GNA_PCI_DEV		0x774c
>  /*
>   * Set power state of select devices that do not have drivers to D3 
> @@ -694,7 +695,7 @@ static void arl_d3_fixup(void)
>  
>  static void arl_h_d3_fixup(void)
>  {
> -	pmc_core_set_device_d3(ARL_NPU_PCI_DEV);
> +	pmc_core_set_device_d3(ARL_H_NPU_PCI_DEV);
>  	pmc_core_set_device_d3(ARL_H_GNA_PCI_DEV);
>  }
>  
> @@ -705,6 +706,13 @@ static int arl_resume(struct pmc_dev *pmcdev)
>  	return cnl_resume(pmcdev);
>  }
>  
> +static void arl_h_suspend(struct pmc_dev *pmcdev) {
> +	arl_h_d3_fixup();
> +
> +	cnl_suspend(pmcdev);
> +}
> +
>  static int arl_h_resume(struct pmc_dev *pmcdev)  {
>  	arl_h_d3_fixup();
> @@ -739,7 +747,7 @@ struct pmc_dev_info arl_h_pmc_dev = {
>  	.dmu_guid = ARL_PMT_DMU_GUID,
>  	.regmap_list = arl_pmc_info_list,
>  	.map = &mtl_socm_reg_map,
> -	.suspend = cnl_suspend,
> +	.suspend = arl_h_suspend,
>  	.resume = arl_h_resume,
>  	.init = arl_h_core_init,
>  };
> 

--
 i.

Re: [PATCH] platform/x86/intel/pmc Fix Arrow Lake U/H support to intel_pmc_core driver

Posted by Brandt, Todd E 7 months, 1 week ago

On Fri, 2025-05-16 at 14:40 +0300, Ilpo Järvinen wrote:
> On Thu, 15 May 2025, Todd Brandt wrote:
> 
> > The ARL requires that the GMA and NPU devices both be in D3Hot in
> > order
> > for PC10 and S0iX to be achieved in S2idle. The original ARL-H/U
> > addition
> > to the intel_pmc_core driver attempted to do this by switching them
> > to D3
> > in the init and resume calls of the intel_pmc_core driver.
> > 
> > The problem is the ARL-H/U have a different NPU device and thus are
> > not
> > being properly set and thus S0iX does not work properly in ARL-H/U.
> > This
> > patch creates a new ARL-H specific device id that is correct and
> > also
> > adds the D3 fixup to the suspend callback. This way if the PCI
> > devies
> > drop from D3 to D0 after resume they can be corrected for the next
> > suspend. Thus there is no dropout in S0iX.
> > 
> 
> Shouldn't this have a Fixes tag?

Yes, you're right, let me re-send it. 

> 
> > Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> > ---
> >  drivers/platform/x86/intel/pmc/arl.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/arl.c
> > b/drivers/platform/x86/intel/pmc/arl.c
> > index 320993bd6d31..5053e3dd8f5e 100644
> > --- a/drivers/platform/x86/intel/pmc/arl.c
> > +++ b/drivers/platform/x86/intel/pmc/arl.c
> > @@ -681,6 +681,7 @@ static struct pmc_info arl_pmc_info_list[] = {
> >  
> >  #define ARL_NPU_PCI_DEV                        0xad1d
> >  #define ARL_GNA_PCI_DEV                        0xae4c
> > +#define ARL_H_NPU_PCI_DEV              0x7d1d
> >  #define ARL_H_GNA_PCI_DEV              0x774c
> >  /*
> >   * Set power state of select devices that do not have drivers to
> > D3
> > @@ -694,7 +695,7 @@ static void arl_d3_fixup(void)
> >  
> >  static void arl_h_d3_fixup(void)
> >  {
> > -       pmc_core_set_device_d3(ARL_NPU_PCI_DEV);
> > +       pmc_core_set_device_d3(ARL_H_NPU_PCI_DEV);
> >         pmc_core_set_device_d3(ARL_H_GNA_PCI_DEV);
> >  }
> >  
> > @@ -705,6 +706,13 @@ static int arl_resume(struct pmc_dev *pmcdev)
> >         return cnl_resume(pmcdev);
> >  }
> >  
> > +static void arl_h_suspend(struct pmc_dev *pmcdev)
> > +{
> > +       arl_h_d3_fixup();
> > +
> > +       cnl_suspend(pmcdev);
> > +}
> > +
> >  static int arl_h_resume(struct pmc_dev *pmcdev)
> >  {
> >         arl_h_d3_fixup();
> > @@ -739,7 +747,7 @@ struct pmc_dev_info arl_h_pmc_dev = {
> >         .dmu_guid = ARL_PMT_DMU_GUID,
> >         .regmap_list = arl_pmc_info_list,
> >         .map = &mtl_socm_reg_map,
> > -       .suspend = cnl_suspend,
> > +       .suspend = arl_h_suspend,
> >         .resume = arl_h_resume,
> >         .init = arl_h_core_init,
> >  };
> > 
>