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

[Winiarska, Iwona]

On Sun, 2026-09-13 at 04:07 +0000, 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.

Similar to the cputemp, it doesn't work because we don't support domains.
For GNR, the IMCs live in a different domain, attempting to query this register
using domain 0 (which is the case for all PECI requests right now - as we're not
filling this field) will likely cause the behavior you're observing.

Thanks
-Iwona

> 
> > > The minimum PECI revision required for GNR is 0x40.
> > > 
> > > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/hwmon/peci/dimmtemp.c | 36
> > +++++++++++++++++++++++++++++++----
> > >   1 file changed, 32 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/peci/dimmtemp.c
> > > b/drivers/hwmon/peci/dimmtemp.c index bd3e8715dfec..9e98effeb928
> > > 100644
> > > --- a/drivers/hwmon/peci/dimmtemp.c
> > > +++ b/drivers/hwmon/peci/dimmtemp.c
> > > @@ -34,8 +34,10 @@
> > >   #define DIMM_IDX_MAX_ON_SPR	2
> > >   #define CHAN_RANK_MAX_ON_EMR	8
> > >   #define DIMM_IDX_MAX_ON_EMR	2
> > > +#define CHAN_RANK_MAX_ON_GNR	12
> > > +#define DIMM_IDX_MAX_ON_GNR	2
> > > 
> > > -#define CHAN_RANK_MAX		CHAN_RANK_MAX_ON_HSX
> > > +#define CHAN_RANK_MAX		CHAN_RANK_MAX_ON_GNR
> > >   #define DIMM_IDX_MAX		DIMM_IDX_MAX_ON_HSX
> > >   #define DIMM_NUMS_MAX		(CHAN_RANK_MAX *
> > DIMM_IDX_MAX)
> > > 
> > > @@ -125,6 +127,9 @@ static int update_thresholds(struct peci_dimmtemp
> > *priv, int dimm_no)
> > >   	if
> > (!peci_sensor_need_update(&priv->dimm[dimm_no].thresholds.state))
> > >   		return 0;
> > > 
> > > +	if (!priv->gen_info->read_thresholds)
> > > +		return -EOPNOTSUPP;
> > > +
> > 
> > This check is unnecessary: The attributes are marked as not visible if
> > read_thresholds is NULL, and this code will never be called.
> 
> Will drop it.
> 
> Best Regards,
> Changhuang