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

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

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.

> The minimum PECI revision required for GNR is 0x40.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>   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.

Thanks,
Guenter