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

[Xi Pardee <xi.pardee@linux.intel.com>]

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!!