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

[Changhuang Liang]

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.

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