Re: [PATCH v1 6/6] hwmon: (peci/dimmtemp) Add support for Granite Rapids (GNR)

[Guenter Roeck <linux@roeck-us.net>]

On 9/12/26 21:07, Changhuang Liang wrote:
> Hi, Guenter
> 
> Thanks for the review.
> 
>> On 9/3/26 06:34, Changhuang Liang wrote:
>>> Add support for Granite Rapids (GNR) platform in PECI DIMM temperature
>>> monitoring driver.
>>>
>>> The GNR platform has different DIMM topology from previous
>>> generations, with 12 channel ranks (CHAN_RANK_MAX_ON_GNR) and 2
>> DIMM
>>> indexes per channel. Define these new constants and update
>>> CHAN_RANK_MAX to use the GNR value since it represents the maximum
>>> across all supported platforms.
>>>
>>> I am not sure whether it differs from previous models, but on GNR,
>>> requests to read DIMM temperature thresholds (DIMM_TEMP_MAX/
>>> DIMM_TEMP_CRIT) via PECI are rejected with completion code 0x90
>>> (invalid request). To handle this, the read_thresholds callback is not
>>> defined for GNR, and the visibility logic is updated to skip exposing
>>> the max and crit temperature attributes when thresholds are not
>>> supported.
>>>
>>
>> Are you sure this doesn't just require different parameter conversion when
>> calling peci_pci_local_read() ? The translation from dimm_order and
>> chan_rank to the register number seems to be different for each architecture.
>>
> 
> Yes, different models have different configuration parameters. The following commit has a reference for this part.
> 
> https://github.com/ocp-hm-openbmc-opf-ami/linux/commit/f851a79143556f0c138617d68e924071434d8bb7
> 
> However, after my actual testing, I found that this logic would always get an invalid request, causing it to fall
> through to the statement below:
> /* Use default or previous value */,
> 
> and in the end, it actually never obtained valid data from the peci client.
> 
> So in my current test environment, I'll temporarily regard this part as dead code and not include it in this series.
> It would be great if Iwona could help confirm this. This also needs to be improved later by someone who
> understands this CPU model better.
> 

I think it would be useful to clarify in the commit message
that this is not a platform limitation but a driver limitation.
As curently written, the commit messages for this and the previous
patch suggest that the platform does not support it, which
based on the available information is incorrect: The problem is
(or appears to be) that it is unknown how to extract the information,
not that it isn't supported by the platform.

Thanks,
Guenter