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

[Guenter Roeck]

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