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

[Changhuang Liang]

Hi, Guenter

> 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/f851a79143556f
> 0
> > c138617d68e924071434d8bb7
> >
> > 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.

Okay, I'll update this commit message.

Best Regards,
Changhuang