[PATCH 0/4] Some TGL and overall S0ix debug improvements

[PATCH 0/4] Some TGL and overall S0ix debug improvements

[Guilherme G. Piccoli]

In this series, we try to improve a bit the debugging of s0ix-related
failures, specially on Tiger Lake platforms.

First patch is a simple clean-up, while patches 2 and 3 attempt to dump
more information on failure cases. For patch 3, it would be good to have
maintainers' validation that we can indeed dump the LPM registers in both
fail paths, as proposed here.

Now, the most controversial one is patch 4, sent as RFC: it effectively
reverts a commit that previously dropped SLP_Sx_DBG register dump on
TGL s0ix-failures. It mentions sub-states as a reason, but without
details. Questions that remain: is it the case that all TGL CPUs have
this limitation, or only some of them? If some of them, can/should we
filter them instead of suppressing this debug info for all Tiger Lake
CPUs? Also, what is the con in dumping this register, is it just
potential bogus values or this could cause an impact on successful
suspend path?

Thanks in advance for reviews!
Cheers,


Guilherme


Guilherme G. Piccoli (4):
  platform/x86/intel/pmc: Fix typo on CNP register name (and clarify comment)
  platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them
  platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths
  [RFC] platform/x86/intel/pmc: Re-add SLP_S0_DBG register dump on Tiger Lake

 drivers/platform/x86/intel/pmc/cnp.c  |  2 +-
 drivers/platform/x86/intel/pmc/core.c | 37 ++++++++++++++-------
 drivers/platform/x86/intel/pmc/core.h |  2 +-
 drivers/platform/x86/intel/pmc/tgl.c  | 48 +++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 14 deletions(-)

-- 
2.50.1

[PATCH 1/4] platform/x86/intel/pmc: Fix typo on CNP register name (and clarify comment)

[Guilherme G. Piccoli]

On SLP_S0_DBG register mapping, s/ALST/ASLT, as in "Aggregated System
Latency" [0]. Also, just clarify that the PMC register offsets are not
only for Cannon Lake, since many other uarchs are using them.

[0] One can check in code, other places use ASLT, but also refer
to the following spec:
Intel 500 Series Chipset Family PCH datasheet - Vol 2
(Doc ID: 636174). Link (from Sep/2025):
www.intel.com/content/www/us/en/content-details/636174/intel-500-series-chipset-family-platform-controller-hub-pch-datasheet-volume-2-of-2.html

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/platform/x86/intel/pmc/cnp.c  | 2 +-
 drivers/platform/x86/intel/pmc/core.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c
index efea4e1ba52b..cf8927ed48db 100644
--- a/drivers/platform/x86/intel/pmc/cnp.c
+++ b/drivers/platform/x86/intel/pmc/cnp.c
@@ -142,7 +142,7 @@ static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
 	{"CNV_VNN_REQ_ACT",	BIT(10)},
 	{"NPK_VNNON_REQ_ACT",	BIT(11)},
 	{"PMSYNC_STATE_IDLE",	BIT(12)},
-	{"ALST_GT_THRES",	BIT(13)},
+	{"ASLT_GT_THRES",	BIT(13)},
 	{"PMC_ARC_PG_READY",	BIT(14)},
 	{}
 };
diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
index 4a94a4ee031e..85677bdc3b1c 100644
--- a/drivers/platform/x86/intel/pmc/core.h
+++ b/drivers/platform/x86/intel/pmc/core.h
@@ -153,7 +153,7 @@ enum ppfear_regs {
 #define SPT_PMC_VRIC1_SLPS0LVEN			BIT(13)
 #define SPT_PMC_VRIC1_XTALSDQDIS		BIT(22)
 
-/* Cannonlake Power Management Controller register offsets */
+/* Cannonlake (and others) Power Management Controller register offsets */
 #define CNP_PMC_SLPS0_DBG_OFFSET		0x10B4
 #define CNP_PMC_PM_CFG_OFFSET			0x1818
 #define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET	0x193C
-- 
2.50.1

[PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Guilherme G. Piccoli]

Right now, SLP_Sx_DBG registers output only show matching bits according
to the register maps and do not distinguish between the different offsets
(SLP_S0_DBG, SLP_S1_DBG, etc).

Let's dump the full register read (like the LPM output does), and
show the id of register to help matching with specs.

This should bring no functional change, the goal is only to improve
reading and allow full comparison between raw register values.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/platform/x86/intel/pmc/core.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index d040290e80ff..c8ce5d6ec30c 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -226,24 +226,31 @@ static void pmc_core_slps0_display(struct pmc *pmc, struct device *dev,
 	const struct pmc_bit_map **maps = pmc->map->slps0_dbg_maps;
 	const struct pmc_bit_map *map;
 	int offset = pmc->map->slps0_dbg_offset;
+	u8 cnt = 0;
 	u32 data;
 
 	while (*maps) {
 		map = *maps;
 		data = pmc_core_reg_read(pmc, offset);
 		offset += 4;
+
+		if (dev)
+			dev_info(dev, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
+		if (s)
+			seq_printf(s, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
 		while (map->name) {
 			if (dev)
-				dev_info(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
-					map->name,
+				dev_info(dev, "SLP_S%u_DBG: %-32s\tState: %s\n",
+					cnt, map->name,
 					data & map->bit_mask ? "Yes" : "No");
 			if (s)
-				seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
-					   map->name,
+				seq_printf(s, "SLP_S%u_DBG: %-32s\tState: %s\n",
+					   cnt, map->name,
 					   data & map->bit_mask ? "Yes" : "No");
 			++map;
 		}
 		++maps;
+		++cnt;
 	}
 }
 
-- 
2.50.1

[PATCH 3/4] platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths

[Guilherme G. Piccoli]

Right now, there are 2 fail paths on pmc_core_resume_common(): either
after (some) package(s) didn't enter Cx state, or after s0ix was
not successfully entered.

The code has a debug output, dumping LPM registers, but *only*
on s0ix fail path, not when packages fail to enter some Cx state.

Let's make it output the LPM registers in both fail cases, in order to
help debugging issues.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/platform/x86/intel/pmc/core.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
index c8ce5d6ec30c..aeb5e47cf5bb 100644
--- a/drivers/platform/x86/intel/pmc/core.c
+++ b/drivers/platform/x86/intel/pmc/core.c
@@ -1882,16 +1882,22 @@ int pmc_core_resume_common(struct pmc_dev *pmcdev)
 					 msr_map[i].name, pc_cnt);
 			}
 		}
-		return 0;
+	} else {
+		/* The real interesting case - S0ix failed - lets ask PMC why. */
+		dev_warn(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
+			 pmcdev->s0ix_counter);
+
+		/*
+		 * Notice that SLP_S0_DBG regs are captured on C10 entry,
+		 * according to the spec. So if we didn't enter C10 (i.e.,
+		 * the  above if-block was executed) seems to make no sense
+		 * in dumping them.
+		 */
+		if (pmc->map->slps0_dbg_maps)
+			pmc_core_slps0_display(pmc, dev, NULL);
+
 	}
 
-	/* The real interesting case - S0ix failed - lets ask PMC why. */
-	dev_warn(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
-		 pmcdev->s0ix_counter);
-
-	if (pmc->map->slps0_dbg_maps)
-		pmc_core_slps0_display(pmc, dev, NULL);
-
 	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
 		struct pmc *pmc = pmcdev->pmcs[i];
 
-- 
2.50.1

[PATCH 4/4][RFC] platform/x86/intel/pmc: Re-add SLP_S0_DBG register dump on Tiger Lake

[Guilherme G. Piccoli]

Commit a018e28f0880 ("platform/x86: intel_pmc_core: Remove slp_s0 attributes from tgl_reg_map")
removed the SLP_Sx_DBG register dump on suspend/resume s0ix-related failures
on Tiger Lake. The mentioned reason was related to potential sub-states.

Let's re-enable the SLP_Sx_DBG register dumping on failures, also fixing
the register mapping (according to the spec[0]) and adding it also to
Tiger Lake H, as a means to improve debug of suspend/resume failures .

If we do have the sub-states, but not in all cases, better to have some
platforms with more debug information than entirely suppress this info.

[0] Refer to: "Intel 500 Series Chipset Family PCH datasheet - Vol 2"
(Doc ID: 636174). Link (from Sep/2025):
www.intel.com/content/www/us/en/content-details/636174/intel-500-series-chipset-family-platform-controller-hub-pch-datasheet-volume-2-of-2.html

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/platform/x86/intel/pmc/tgl.c | 48 ++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
index 02e731ed3391..cdabe9b5c20b 100644
--- a/drivers/platform/x86/intel/pmc/tgl.c
+++ b/drivers/platform/x86/intel/pmc/tgl.c
@@ -185,12 +185,58 @@ static const struct pmc_bit_map *tgl_lpm_maps[] = {
 	NULL
 };
 
+/*
+ * The following SLP_S0_DBG register mappings are based on the
+ * "Intel 500 Series Chipset Family PCH datasheet - Vol 2"
+ * specification (Doc ID: 636174).
+ */
+
+static const struct pmc_bit_map tgl_slps0_dbg0_map[] = {
+	{"AUDIO_D3",		BIT(0)},
+	{"OTG_D3",		BIT(1)},
+	{"XHCI_D3",		BIT(2)},
+	{"LPIO_D3",		BIT(3)},
+	{"SATA_D3",		BIT(5)},
+	{}
+};
+
+static const struct pmc_bit_map tgl_slps0_dbg1_map[] = {
+	{"USB2_PLL_OFF",	BIT(1)},
+	{"AUDIO_PLL_OFF",	BIT(2)},
+	{"MAIN_PLL_OFF",	BIT(4)},
+	{"XOSC_OFF",		BIT(5)},
+	{"PCIE_CLKREQS_OFF",	BIT(7)},
+	{"AUDIO_ROSC_OFF",	BIT(8)},
+	{}
+};
+
+static const struct pmc_bit_map tgl_slps0_dbg2_map[] = {
+	{"HSIO_CORE_GATED",	BIT(0)},
+	{"CSME_GATED",		BIT(1)},
+	{"GBE_NO_LINK",		BIT(4)},
+	{"PCIE_LOW_POWER",	BIT(6)},
+	{"ISH_VNN_REQ_ACT",	BIT(8)},
+	{"CNV_VNN_REQ_ACT",	BIT(10)},
+	{"PMSYNC_STATE_IDLE",	BIT(12)},
+	{"ASLT_GT_THRES",	BIT(13)},
+	{}
+};
+
+const struct pmc_bit_map *tgl_slps0_dbg_maps[] = {
+	tgl_slps0_dbg0_map,
+	tgl_slps0_dbg1_map,
+	tgl_slps0_dbg2_map,
+	NULL
+};
+
 static const struct pmc_reg_map tgl_reg_map = {
 	.pfear_sts = ext_tgl_pfear_map,
 	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
+	.slps0_dbg_maps = tgl_slps0_dbg_maps,
 	.slp_s0_res_counter_step = TGL_PMC_SLP_S0_RES_COUNTER_STEP,
 	.ltr_show_sts = cnp_ltr_show_map,
 	.msr_sts = msr_map,
+	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
 	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = CNP_PMC_MMIO_REG_LEN,
 	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
@@ -213,9 +259,11 @@ static const struct pmc_reg_map tgl_reg_map = {
 static const struct pmc_reg_map tgl_h_reg_map = {
 	.pfear_sts = ext_tgl_pfear_map,
 	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
+	.slps0_dbg_maps = tgl_slps0_dbg_maps,
 	.slp_s0_res_counter_step = TGL_PMC_SLP_S0_RES_COUNTER_STEP,
 	.ltr_show_sts = cnp_ltr_show_map,
 	.msr_sts = msr_map,
+	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
 	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
 	.regmap_length = CNP_PMC_MMIO_REG_LEN,
 	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
-- 
2.50.1

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Ilpo Järvinen]

On Mon, 22 Sep 2025, Guilherme G. Piccoli wrote:

> Right now, SLP_Sx_DBG registers output only show matching bits according
> to the register maps and do not distinguish between the different offsets
> (SLP_S0_DBG, SLP_S1_DBG, etc).
> 
> Let's dump the full register read (like the LPM output does), and
> show the id of register to help matching with specs.
> 
> This should bring no functional change, the goal is only to improve
> reading and allow full comparison between raw register values.

Hi,

I don't think that's exactly the definition of "no function change" if you 
intentionally make a change to the reading. :-)

> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  drivers/platform/x86/intel/pmc/core.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index d040290e80ff..c8ce5d6ec30c 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -226,24 +226,31 @@ static void pmc_core_slps0_display(struct pmc *pmc, struct device *dev,
>  	const struct pmc_bit_map **maps = pmc->map->slps0_dbg_maps;
>  	const struct pmc_bit_map *map;
>  	int offset = pmc->map->slps0_dbg_offset;
> +	u8 cnt = 0;
>  	u32 data;
>  
>  	while (*maps) {
>  		map = *maps;
>  		data = pmc_core_reg_read(pmc, offset);
>  		offset += 4;
> +
> +		if (dev)
> +			dev_info(dev, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
> +		if (s)
> +			seq_printf(s, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>  		while (map->name) {
>  			if (dev)
> -				dev_info(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> -					map->name,
> +				dev_info(dev, "SLP_S%u_DBG: %-32s\tState: %s\n",

I'm not sure about this change. To me it looks the naming is "SLP S0 DEBUG 
REGx (SLP_S0_DBG_x)" according to this:

https://edc.intel.com/content/www/tw/zh/design/publications/14th-generation-core-processors-ioe-p-registers/slp-s0-debug-reg2-slp-s0-dbg-2-offset-10bc/

...So changing from S0 to S1 or S2 does not seem correct here?

I wonder if this really a problem to begin with as the names should be 
unique, no?

> +					cnt, map->name,
>  					data & map->bit_mask ? "Yes" : "No");
>  			if (s)
> -				seq_printf(s, "SLP_S0_DBG: %-32s\tState: %s\n",
> -					   map->name,
> +				seq_printf(s, "SLP_S%u_DBG: %-32s\tState: %s\n",
> +					   cnt, map->name,
>  					   data & map->bit_mask ? "Yes" : "No");
>  			++map;
>  		}
>  		++maps;
> +		++cnt;

This assumption seems somewhat fragile but maybe it's not worth 
engineering it beyond this at this point.


Also, please remember to add all maintainers as receipients when posting.



-- 
 i.

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Guilherme G. Piccoli]

Hi Ilpo, thanks a lot for your review, very nice points! Comments below:


On 23/09/2025 04:59, Ilpo Järvinen wrote:
> [...]
>> This should bring no functional change, the goal is only to improve
>> reading and allow full comparison between raw register values.
> 
> Hi,
> 
> I don't think that's exactly the definition of "no function change" if you 
> intentionally make a change to the reading. :-)
>

Hehe yeah, you're right - it changes the output, so that's a functional
change indeed (imagine a userspace script parsing it...). I was thinking
in functional change as in "no extra register reads are performed", but
I agree with you and will drop this text from next version, thanks for
pointing!


>[...]
>> +
>> +		if (dev)
>> +			dev_info(dev, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>> +		if (s)
>> +			seq_printf(s, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>>  		while (map->name) {
>>  			if (dev)
>> -				dev_info(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
>> -					map->name,
>> +				dev_info(dev, "SLP_S%u_DBG: %-32s\tState: %s\n",
> 
> I'm not sure about this change. To me it looks the naming is "SLP S0 DEBUG 
> REGx (SLP_S0_DBG_x)" according to this:
> 
> https://edc.intel.com/content/www/tw/zh/design/publications/14th-generation-core-processors-ioe-p-registers/slp-s0-debug-reg2-slp-s0-dbg-2-offset-10bc/
> 
> ...So changing from S0 to S1 or S2 does not seem correct here?
> 
> I wonder if this really a problem to begin with as the names should be 
> unique, no?

Totally right again! Nice catch, it should be as you pointed, the
different ID is at the end of the string.
And no, it's definitely not a problem / big deal - I just took the
opportunity to improve the output, but I messed up heh

Lemme know if you prefer that I keep the old naming or fix it properly,
like SLP_S0_DBG_2, etc.


> [...]
> This assumption seems somewhat fragile but maybe it's not worth 
> engineering it beyond this at this point.

Sorry, I couldn't understand this sentence. Can you clarify it for me?
What assumption and what do you think we should do?


>[...] 
> Also, please remember to add all maintainers as receipients when posting.

My apologies - I checked MAINTAINERS directly and added everyone from
INTEL PMC + Xi Pardee (many patches in the driver); but I should have
used get_maintainers instead, which brings your name. Thanks for
reviewing even when I forgot to add your email!!
Cheers,


Guilherme

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Ilpo Järvinen]

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

On Tue, 23 Sep 2025, Guilherme G. Piccoli wrote:

> Hi Ilpo, thanks a lot for your review, very nice points! Comments below:
> 
> 
> On 23/09/2025 04:59, Ilpo Järvinen wrote:
> > [...]
> >> This should bring no functional change, the goal is only to improve
> >> reading and allow full comparison between raw register values.
> > 
> > Hi,
> > 
> > I don't think that's exactly the definition of "no function change" if you 
> > intentionally make a change to the reading. :-)
> 
> Hehe yeah, you're right - it changes the output, so that's a functional
> change indeed (imagine a userspace script parsing it...). I was thinking
> in functional change as in "no extra register reads are performed", but
> I agree with you and will drop this text from next version, thanks for
> pointing!
> 
> 
> >[...]
> >> +
> >> +		if (dev)
> >> +			dev_info(dev, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
> >> +		if (s)
> >> +			seq_printf(s, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
> >>  		while (map->name) {
> >>  			if (dev)
> >> -				dev_info(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
> >> -					map->name,
> >> +				dev_info(dev, "SLP_S%u_DBG: %-32s\tState: %s\n",
> > 
> > I'm not sure about this change. To me it looks the naming is "SLP S0 DEBUG 
> > REGx (SLP_S0_DBG_x)" according to this:
> > 
> > https://edc.intel.com/content/www/tw/zh/design/publications/14th-generation-core-processors-ioe-p-registers/slp-s0-debug-reg2-slp-s0-dbg-2-offset-10bc/
> > 
> > ...So changing from S0 to S1 or S2 does not seem correct here?
> > 
> > I wonder if this really a problem to begin with as the names should be 
> > unique, no?
> 
> Totally right again! Nice catch, it should be as you pointed, the
> different ID is at the end of the string.
> And no, it's definitely not a problem / big deal - I just took the
> opportunity to improve the output, but I messed up heh
> 
> Lemme know if you prefer that I keep the old naming or fix it properly,
> like SLP_S0_DBG_2, etc.

I'd prefer Xi or David comment on this whether to add the number there or 
not. This will end up being after the merge window material anyway so lets 
give them a few days.

> > [...]
> > This assumption seems somewhat fragile but maybe it's not worth 
> > engineering it beyond this at this point.
> 
> Sorry, I couldn't understand this sentence. Can you clarify it for me?
> What assumption and what do you think we should do?

I was just referring to the ++ line that you for some reason snipped. 
It assumed certain order of things in the input array which arms a 
trap. But then, we know all the current inputs are okay with this simple 
approach and I'm not sure if this code is getting much changes in the 
future so it might be over-engineering to store the number into the 
input array within the struct.

> >[...] 
> > Also, please remember to add all maintainers as receipients when posting.
> 
> My apologies - I checked MAINTAINERS directly and added everyone from
> INTEL PMC + Xi Pardee (many patches in the driver); but I should have
> used get_maintainers instead, which brings your name. Thanks for
> reviewing even when I forgot to add your email!!

-- 
 i.

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Guilherme G. Piccoli]

On 24/09/2025 06:57, Ilpo Järvinen wrote:
> [...]
>> Lemme know if you prefer that I keep the old naming or fix it properly,
>> like SLP_S0_DBG_2, etc.
> 
> I'd prefer Xi or David comment on this whether to add the number there or 
> not. This will end up being after the merge window material anyway so lets 
> give them a few days.
> 

Perfect for me, thanks!


>> Sorry, I couldn't understand this sentence. Can you clarify it for me?
>> What assumption and what do you think we should do?
> 
> I was just referring to the ++ line that you for some reason snipped. 
> It assumed certain order of things in the input array which arms a 
> trap. But then, we know all the current inputs are okay with this simple 
> approach and I'm not sure if this code is getting much changes in the 
> future so it might be over-engineering to store the number into the 
> input array within the struct.
> 

OK, thanks for the clarification. Let's see the input from others and we
take it from there =)

Re: [PATCH 0/4] Some TGL and overall S0ix debug improvements

[Guilherme G. Piccoli]

On 22/09/2025 19:52, Guilherme G. Piccoli wrote:
> In this series, we try to improve a bit the debugging of s0ix-related
> failures, specially on Tiger Lake platforms.
> 
> First patch is a simple clean-up, while patches 2 and 3 attempt to dump
> more information on failure cases. For patch 3, it would be good to have
> maintainers' validation that we can indeed dump the LPM registers in both
> fail paths, as proposed here.
> 
> Now, the most controversial one is patch 4, sent as RFC: it effectively
> reverts a commit that previously dropped SLP_Sx_DBG register dump on
> TGL s0ix-failures. It mentions sub-states as a reason, but without
> details. Questions that remain: is it the case that all TGL CPUs have
> this limitation, or only some of them? If some of them, can/should we
> filter them instead of suppressing this debug info for all Tiger Lake
> CPUs? Also, what is the con in dumping this register, is it just
> potential bogus values or this could cause an impact on successful
> suspend path?
> 
> Thanks in advance for reviews!
> Cheers,
> 
> 
> Guilherme
> 
> 
> Guilherme G. Piccoli (4):
>   platform/x86/intel/pmc: Fix typo on CNP register name (and clarify comment)
>   platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them
>   platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths
>   [RFC] platform/x86/intel/pmc: Re-add SLP_S0_DBG register dump on Tiger Lake
> 

Hi folks, gentle ping.
If you prefer, I can also resend, rebased on top of Linus latest tree.

Cheers,


Guilherme

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Xi Pardee]

Hi,

Sorry for the late response. My response is inline.

Xi

On 9/24/2025 2:57 AM, Ilpo Järvinen wrote:
> On Tue, 23 Sep 2025, Guilherme G. Piccoli wrote:
>
>> Hi Ilpo, thanks a lot for your review, very nice points! Comments below:
>>
>>
>> On 23/09/2025 04:59, Ilpo Järvinen wrote:
>>> [...]
>>>> This should bring no functional change, the goal is only to improve
>>>> reading and allow full comparison between raw register values.
>>> Hi,
>>>
>>> I don't think that's exactly the definition of "no function change" if you
>>> intentionally make a change to the reading. :-)
>> Hehe yeah, you're right - it changes the output, so that's a functional
>> change indeed (imagine a userspace script parsing it...). I was thinking
>> in functional change as in "no extra register reads are performed", but
>> I agree with you and will drop this text from next version, thanks for
>> pointing!
>>
>>
>>> [...]
>>>> +
>>>> +		if (dev)
>>>> +			dev_info(dev, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>>>> +		if (s)
>>>> +			seq_printf(s, "\nSLP_S%u_DBG:\t0x%x\n", cnt, data);
>>>>   		while (map->name) {
>>>>   			if (dev)
>>>> -				dev_info(dev, "SLP_S0_DBG: %-32s\tState: %s\n",
>>>> -					map->name,
>>>> +				dev_info(dev, "SLP_S%u_DBG: %-32s\tState: %s\n",
>>> I'm not sure about this change. To me it looks the naming is "SLP S0 DEBUG
>>> REGx (SLP_S0_DBG_x)" according to this:
>>>
>>> https://edc.intel.com/content/www/tw/zh/design/publications/14th-generation-core-processors-ioe-p-registers/slp-s0-debug-reg2-slp-s0-dbg-2-offset-10bc/
>>>
>>> ...So changing from S0 to S1 or S2 does not seem correct here?
>>>
>>> I wonder if this really a problem to begin with as the names should be
>>> unique, no?
>> Totally right again! Nice catch, it should be as you pointed, the
>> different ID is at the end of the string.
>> And no, it's definitely not a problem / big deal - I just took the
>> opportunity to improve the output, but I messed up heh
>>
>> Lemme know if you prefer that I keep the old naming or fix it properly,
>> like SLP_S0_DBG_2, etc.
> I'd prefer Xi or David comment on this whether to add the number there or
> not. This will end up being after the merge window material anyway so lets
> give them a few days.

I am still uncertain about the added value of this patch. Could you 
please elaborate on how displaying the register name would assist with 
debugging?

The registers in slps0_dbg_maps are intended for diagnosing slps0 
related issues, and the register names should follow the format 
SLP_S0_DBG_(1,2,3).

Additionally, the full 32-bit register data should not be displayed 
here, as some of the bits are reserved and must not be exposed to users.

>>> [...]
>>> This assumption seems somewhat fragile but maybe it's not worth
>>> engineering it beyond this at this point.
>> Sorry, I couldn't understand this sentence. Can you clarify it for me?
>> What assumption and what do you think we should do?
> I was just referring to the ++ line that you for some reason snipped.
> It assumed certain order of things in the input array which arms a
> trap. But then, we know all the current inputs are okay with this simple
> approach and I'm not sure if this code is getting much changes in the
> future so it might be over-engineering to store the number into the
> input array within the struct.
>
>>> [...]
>>> Also, please remember to add all maintainers as receipients when posting.
>> My apologies - I checked MAINTAINERS directly and added everyone from
>> INTEL PMC + Xi Pardee (many patches in the driver); but I should have
>> used get_maintainers instead, which brings your name. Thanks for
>> reviewing even when I forgot to add your email!!

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Guilherme G. Piccoli]

Hi Xi, thanks for you feedback. Comments inline:

On 13/10/2025 14:39, Xi Pardee wrote:
> [...]

> I am still uncertain about the added value of this patch. Could you 
> please elaborate on how displaying the register name would assist with 
> debugging?
> 
> The registers in slps0_dbg_maps are intended for diagnosing slps0 
> related issues, and the register names should follow the format 
> SLP_S0_DBG_(1,2,3).
> 

The names do not help with debugging issues, definitely. It's just a
detail, an improvement - or at least, I consider the output a bit better
with this patch. But that part is superfluous, if you prefer not to
change it, I'm totally OK. And ofc you / Ilpo are right, I changed the
naming in a wrong way, I'd fix that in next iteration (thanks for noticing).


> Additionally, the full 32-bit register data should not be displayed 
> here, as some of the bits are reserved and must not be exposed to users.
> 

Now, this one I respectfully disagree - these raw values have absolutely
no meaning for the end-user, exactly due to the reason you said: there
are many reserved bits. So, for a normal user, a piece of hex values,
junk...

But for Intel, it might be the case you do have internal specs detailing
these reserved bits, this could be meaningful. That's exactly the
purpose of this patch: when debugging rare to reproduce failures, having
this raw register could help Intel to determine the cause of odd
failures and eventually fix it. No harm (or meaning) for end-users though.

Of course the driver is from Intel, and as maintainers you could decide
to not show the raw debug data from these registers - fine as well, let
me know what you prefer.

Thanks,


Guilherme

Re: [PATCH 4/4][RFC] platform/x86/intel/pmc: Re-add SLP_S0_DBG register dump on Tiger Lake

[Xi Pardee]

Hi,

My response is in line.

Thanks!
Xi

On 9/22/2025 3:52 PM, Guilherme G. Piccoli wrote:
> Commit a018e28f0880 ("platform/x86: intel_pmc_core: Remove slp_s0 attributes from tgl_reg_map")
> removed the SLP_Sx_DBG register dump on suspend/resume s0ix-related failures
> on Tiger Lake. The mentioned reason was related to potential sub-states.
>
> Let's re-enable the SLP_Sx_DBG register dumping on failures, also fixing
> the register mapping (according to the spec[0]) and adding it also to
> Tiger Lake H, as a means to improve debug of suspend/resume failures .
>
> If we do have the sub-states, but not in all cases, better to have some
> platforms with more debug information than entirely suppress this info.
>
> [0] Refer to: "Intel 500 Series Chipset Family PCH datasheet - Vol 2"
> (Doc ID: 636174). Link (from Sep/2025):
> www.intel.com/content/www/us/en/content-details/636174/intel-500-series-chipset-family-platform-controller-hub-pch-datasheet-volume-2-of-2.html
>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Starting with Tiger Lake platforms, the slp_s0_dbg register maps are 
deprecated. The data provided by these registers may no longer be valid.

On Tiger Lake and newer platforms, the intel_pmc_core driver introduces 
substate_status_registers and other updated attributes that offer 
information useful for debugging S0ix-related issues.

> ---
>   drivers/platform/x86/intel/pmc/tgl.c | 48 ++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
>
> diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
> index 02e731ed3391..cdabe9b5c20b 100644
> --- a/drivers/platform/x86/intel/pmc/tgl.c
> +++ b/drivers/platform/x86/intel/pmc/tgl.c
> @@ -185,12 +185,58 @@ static const struct pmc_bit_map *tgl_lpm_maps[] = {
>   	NULL
>   };
>   
> +/*
> + * The following SLP_S0_DBG register mappings are based on the
> + * "Intel 500 Series Chipset Family PCH datasheet - Vol 2"
> + * specification (Doc ID: 636174).
> + */
> +
> +static const struct pmc_bit_map tgl_slps0_dbg0_map[] = {
> +	{"AUDIO_D3",		BIT(0)},
> +	{"OTG_D3",		BIT(1)},
> +	{"XHCI_D3",		BIT(2)},
> +	{"LPIO_D3",		BIT(3)},
> +	{"SATA_D3",		BIT(5)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map tgl_slps0_dbg1_map[] = {
> +	{"USB2_PLL_OFF",	BIT(1)},
> +	{"AUDIO_PLL_OFF",	BIT(2)},
> +	{"MAIN_PLL_OFF",	BIT(4)},
> +	{"XOSC_OFF",		BIT(5)},
> +	{"PCIE_CLKREQS_OFF",	BIT(7)},
> +	{"AUDIO_ROSC_OFF",	BIT(8)},
> +	{}
> +};
> +
> +static const struct pmc_bit_map tgl_slps0_dbg2_map[] = {
> +	{"HSIO_CORE_GATED",	BIT(0)},
> +	{"CSME_GATED",		BIT(1)},
> +	{"GBE_NO_LINK",		BIT(4)},
> +	{"PCIE_LOW_POWER",	BIT(6)},
> +	{"ISH_VNN_REQ_ACT",	BIT(8)},
> +	{"CNV_VNN_REQ_ACT",	BIT(10)},
> +	{"PMSYNC_STATE_IDLE",	BIT(12)},
> +	{"ASLT_GT_THRES",	BIT(13)},
> +	{}
> +};
> +
> +const struct pmc_bit_map *tgl_slps0_dbg_maps[] = {
> +	tgl_slps0_dbg0_map,
> +	tgl_slps0_dbg1_map,
> +	tgl_slps0_dbg2_map,
> +	NULL
> +};
> +
>   static const struct pmc_reg_map tgl_reg_map = {
>   	.pfear_sts = ext_tgl_pfear_map,
>   	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> +	.slps0_dbg_maps = tgl_slps0_dbg_maps,
>   	.slp_s0_res_counter_step = TGL_PMC_SLP_S0_RES_COUNTER_STEP,
>   	.ltr_show_sts = cnp_ltr_show_map,
>   	.msr_sts = msr_map,
> +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>   	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>   	.regmap_length = CNP_PMC_MMIO_REG_LEN,
>   	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> @@ -213,9 +259,11 @@ static const struct pmc_reg_map tgl_reg_map = {
>   static const struct pmc_reg_map tgl_h_reg_map = {
>   	.pfear_sts = ext_tgl_pfear_map,
>   	.slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> +	.slps0_dbg_maps = tgl_slps0_dbg_maps,
>   	.slp_s0_res_counter_step = TGL_PMC_SLP_S0_RES_COUNTER_STEP,
>   	.ltr_show_sts = cnp_ltr_show_map,
>   	.msr_sts = msr_map,
> +	.slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
>   	.ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
>   	.regmap_length = CNP_PMC_MMIO_REG_LEN,
>   	.ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,

Re: [PATCH 3/4] platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths

[Xi Pardee]

Hi,

My response is inline.

Thanks!

Xi

On 9/22/2025 3:52 PM, Guilherme G. Piccoli wrote:
> Right now, there are 2 fail paths on pmc_core_resume_common(): either
> after (some) package(s) didn't enter Cx state, or after s0ix was
> not successfully entered.
>
> The code has a debug output, dumping LPM registers, but *only*
> on s0ix fail path, not when packages fail to enter some Cx state.
>
> Let's make it output the LPM registers in both fail cases, in order to
> help debugging issues.
>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>   drivers/platform/x86/intel/pmc/core.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index c8ce5d6ec30c..aeb5e47cf5bb 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -1882,16 +1882,22 @@ int pmc_core_resume_common(struct pmc_dev *pmcdev)
>   					 msr_map[i].name, pc_cnt);
>   			}
>   		}
> -		return 0;
> +	} else {
> +		/* The real interesting case - S0ix failed - lets ask PMC why. */
> +		dev_warn(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
> +			 pmcdev->s0ix_counter);
> +
> +		/*
> +		 * Notice that SLP_S0_DBG regs are captured on C10 entry,
> +		 * according to the spec. So if we didn't enter C10 (i.e.,
> +		 * the  above if-block was executed) seems to make no sense
> +		 * in dumping them.
> +		 */
> +		if (pmc->map->slps0_dbg_maps)
> +			pmc_core_slps0_display(pmc, dev, NULL);
> +
>   	}
>   
> -	/* The real interesting case - S0ix failed - lets ask PMC why. */
> -	dev_warn(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
> -		 pmcdev->s0ix_counter);
> -
> -	if (pmc->map->slps0_dbg_maps)
> -		pmc_core_slps0_display(pmc, dev, NULL);
> -
>   	for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
>   		struct pmc *pmc = pmcdev->pmcs[i];
>   

Entering the S0ix state requires the system to reach the deepest package 
C-state, PC10. If the system fails to achieve PC10 residency during 
suspend, the slps0_dbg_maps data becomes irrelevant and may mislead 
users. For this reason, the driver hides this information when PC10 
residency is not attained.

Re: [PATCH 3/4] platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths

[Guilherme G. Piccoli]

Thanks Xi! Response inline:

On 14/10/2025 16:29, Xi Pardee wrote:
> [...]
> 
> Entering the S0ix state requires the system to reach the deepest package 
> C-state, PC10. If the system fails to achieve PC10 residency during 
> suspend, the slps0_dbg_maps data becomes irrelevant and may mislead 
> users. For this reason, the driver hides this information when PC10 
> residency is not attained.
> 

Indeed, I agree - this is even mentioned in the spec.

But the patch here doesn't change that (or shouldn't - except if I did
something wrong heh).

The idea of this patch is to dump the LPM registers in any failure path,
the ones printed by pmc_core_lpm_display() - not related with the
slps0_dbg_maps.

Lemme know your thoughts on this.
Cheers,


Guilherme

Re: [PATCH 4/4][RFC] platform/x86/intel/pmc: Re-add SLP_S0_DBG register dump on Tiger Lake

[Guilherme G. Piccoli]

On 14/10/2025 16:24, Xi Pardee wrote:
> [...]
> 
> Starting with Tiger Lake platforms, the slp_s0_dbg register maps are 
> deprecated. The data provided by these registers may no longer be valid.
> 
> On Tiger Lake and newer platforms, the intel_pmc_core driver introduces 
> substate_status_registers and other updated attributes that offer 
> information useful for debugging S0ix-related issues.
> 

Thanks for the clarification Xi.
So, this one is clearly a patch to drop in a potential re-submission!
Cheers,


Guilherme

Re: [PATCH 3/4] platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths

[Xi Pardee]

On 10/14/2025 12:58 PM, Guilherme G. Piccoli wrote:
> Thanks Xi! Response inline:
>
> On 14/10/2025 16:29, Xi Pardee wrote:
>> [...]
>>
>> Entering the S0ix state requires the system to reach the deepest package
>> C-state, PC10. If the system fails to achieve PC10 residency during
>> suspend, the slps0_dbg_maps data becomes irrelevant and may mislead
>> users. For this reason, the driver hides this information when PC10
>> residency is not attained.
>>
> Indeed, I agree - this is even mentioned in the spec.
>
> But the patch here doesn't change that (or shouldn't - except if I did
> something wrong heh).
>
> The idea of this patch is to dump the LPM registers in any failure path,
> the ones printed by pmc_core_lpm_display() - not related with the
> slps0_dbg_maps.
>
> Lemme know your thoughts on this.
> Cheers,
>
>
> Guilherme
When the system fails to enter the S0ix state during suspend, there are 
two scenarios to consider:
1. If the system achieves PC10 residency, debugging S0ix can focus on 
the information provided by slps0_dbg_maps or lpm_sts.
2. If the system does not achieve PC10 residency, the PMC core driver 
supplies package C-state residency information instead.
It is important to note that the data presented by 
pmc_core_lpm_display() is only relevant when PC10 residency has been 
attained.
This difference helps ensure that users are provided with accurate and 
meaningful information relevant to the power state achieved during suspend.
Thanks!
Xi
>

Re: [PATCH 3/4] platform/x86/intel/pmc: Always dump LPM status regs on unsuccessful paths

[Guilherme G. Piccoli]

On 14/10/2025 20:55, Xi Pardee wrote:
> [...]
> When the system fails to enter the S0ix state during suspend, there are 
> two scenarios to consider:
> 1. If the system achieves PC10 residency, debugging S0ix can focus on 
> the information provided by slps0_dbg_maps or lpm_sts.
> 2. If the system does not achieve PC10 residency, the PMC core driver 
> supplies package C-state residency information instead.
> It is important to note that the data presented by 
> pmc_core_lpm_display() is only relevant when PC10 residency has been 
> attained.
> This difference helps ensure that users are provided with accurate and 
> meaningful information relevant to the power state achieved during suspend.
> Thanks!
> Xi
>>

Thanks for the clarification Xi! So, this patch doesn't make sense, in
the end - let's drop it.
With that, both patches 3 and 4 are discarded.

Lemme know if you / other maintainers see any value on patches 1-2 or
else, we can discard the whole series.
Cheers,


Guilherme

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Xi Pardee]

On 10/13/2025 11:05 AM, Guilherme G. Piccoli wrote:
> Hi Xi, thanks for you feedback. Comments inline:
>
> On 13/10/2025 14:39, Xi Pardee wrote:
>> [...]
>> I am still uncertain about the added value of this patch. Could you
>> please elaborate on how displaying the register name would assist with
>> debugging?
>>
>> The registers in slps0_dbg_maps are intended for diagnosing slps0
>> related issues, and the register names should follow the format
>> SLP_S0_DBG_(1,2,3).
>>
> The names do not help with debugging issues, definitely. It's just a
> detail, an improvement - or at least, I consider the output a bit better
> with this patch. But that part is superfluous, if you prefer not to
> change it, I'm totally OK. And ofc you / Ilpo are right, I changed the
> naming in a wrong way, I'd fix that in next iteration (thanks for noticing).
>
>
>> Additionally, the full 32-bit register data should not be displayed
>> here, as some of the bits are reserved and must not be exposed to users.
>>
> Now, this one I respectfully disagree - these raw values have absolutely
> no meaning for the end-user, exactly due to the reason you said: there
> are many reserved bits. So, for a normal user, a piece of hex values,
> junk...
>
> But for Intel, it might be the case you do have internal specs detailing
> these reserved bits, this could be meaningful. That's exactly the
> purpose of this patch: when debugging rare to reproduce failures, having
> this raw register could help Intel to determine the cause of odd
> failures and eventually fix it. No harm (or meaning) for end-users though.
>
> Of course the driver is from Intel, and as maintainers you could decide
> to not show the raw debug data from these registers - fine as well, let
> me know what you prefer.
>
> Thanks,
>
>
> Guilherme

Hi,

We would prefer not to show reserved bits as they don't provide values 
to the users.

Thanks!

Xi

Re: [PATCH 2/4] platform/x86/intel/pmc: Dump raw SLP_Sx_DBG registers and distinguish between them

[Guilherme G. Piccoli]

On 23/10/2025 18:44, Xi Pardee wrote:
> [...]
> Hi,
> 
> We would prefer not to show reserved bits as they don't provide values 
> to the users.
> 
> Thanks!
> 
> Xi
> 

OK Xi, thanks for your review. I guess we can drop this one, no biggies.
Cheers,


Guilherme