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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.