Re: [PATCH v4 55/71] hwmon: (peci/cputemp) Switch to new Intel CPU model defines

["Winiarska, Iwona" <iwona.winiarska@intel.com>]

On Thu, 2024-04-25 at 12:58 -0700, Tony Luck wrote:
> > Is drivers/peci/device.c: peci_get_cpu_id() specific to Intel CPUs? It seems
> > to retrieve a value in the format of CPUID(EAX=1).EAX that would apply to
> > both Intel and AMD. Yet I don't see any checking of vendor id before looking
> > at the model id.
> 
> Some web wearching indicates that PECI is an Intel specific protocol, so
> no need for vendor checks.

Correct.

> 
> I've re-worked the peci patches (combining the core and hwmon bits) to
> switch from separate "family" and "model" fields and checks to using
> the same x86_vfm combined value.
> 
> The peci patch is included below. But you can see it in context in:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git
> new_families_v5_wip
> 
> The x86 build of these bits works. But I don't have cross-compile
> enviroment to check non-X86 builds. Also no way to test.

Compile-tested for now, but it looks good to me in general.
One small comment - the kerneldoc for struct peci_device_id in
drivers/peci/internal.h also needs to be updated.

Thanks
-Iwona

> 
> -Tony
> 
> ---
> 
> From 34cc093cbebaf596a52b123c4f8c3ef2df45a240 Mon Sep 17 00:00:00 2001
> From: Tony Luck <tony.luck@intel.com>
> Date: Thu, 25 Apr 2024 12:27:39 -0700
> Subject: [PATCH] peci, hwmon: Switch to new Intel CPU model defines
> 
> Update peci subsystem to use the same vendor-family-model
> combined definition that core x86 code uses.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/peci-cpu.h     | 24 ++++++++++++++++++++++++
>  include/linux/peci.h         |  3 +--
>  drivers/peci/internal.h      |  3 +--
>  drivers/hwmon/peci/cputemp.c |  8 ++++----
>  drivers/peci/core.c          |  5 ++---
>  drivers/peci/cpu.c           | 21 +++++++--------------
>  drivers/peci/device.c        |  3 +--
>  7 files changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/peci-cpu.h b/include/linux/peci-cpu.h
> index ff8ae9c26c80..601cdd086bf6 100644
> --- a/include/linux/peci-cpu.h
> +++ b/include/linux/peci-cpu.h
> @@ -6,6 +6,30 @@
>  
>  #include <linux/types.h>
>  
> +/* Copied from x86 <asm/processor.h> */
> +#define X86_VENDOR_INTEL       0
> +
> +/* Copied from x86 <asm/cpu_device_id.h> */
> +#define VFM_MODEL_BIT	0
> +#define VFM_FAMILY_BIT	8
> +#define VFM_VENDOR_BIT	16
> +#define VFM_RSVD_BIT	24
> +
> +#define	VFM_MODEL_MASK	GENMASK(VFM_FAMILY_BIT - 1, VFM_MODEL_BIT)
> +#define	VFM_FAMILY_MASK	GENMASK(VFM_VENDOR_BIT - 1,
> VFM_FAMILY_BIT)
> +#define	VFM_VENDOR_MASK	GENMASK(VFM_RSVD_BIT - 1,
> VFM_VENDOR_BIT)
> +
> +#define VFM_MODEL(vfm)	(((vfm) & VFM_MODEL_MASK) >> VFM_MODEL_BIT)
> +#define VFM_FAMILY(vfm)	(((vfm) & VFM_FAMILY_MASK) >> VFM_FAMILY_BIT)
> +#define VFM_VENDOR(vfm)	(((vfm) & VFM_VENDOR_MASK) >> VFM_VENDOR_BIT)
> +
> +#define	VFM_MAKE(_vendor, _family, _model) (	\
> +	((_model) << VFM_MODEL_BIT) |		\
> +	((_family) << VFM_FAMILY_BIT) |		\
> +	((_vendor) << VFM_VENDOR_BIT)		\
> +)
> +/* End of copied code */
> +
>  #include "../../arch/x86/include/asm/intel-family.h"
>  
>  #define PECI_PCS_PKG_ID			0  /* Package Identifier Read
> */
> diff --git a/include/linux/peci.h b/include/linux/peci.h
> index 9b3d36aff431..0db8b5dd09d9 100644
> --- a/include/linux/peci.h
> +++ b/include/linux/peci.h
> @@ -74,8 +74,7 @@ static inline struct peci_controller
> *to_peci_controller(void *d)
>  struct peci_device {
>  	struct device dev;
>  	struct {
> -		u16 family;
> -		u8 model;
> +		u32 x86_vfm;
>  		u8 peci_revision;
>  		u8 socket_id;
>  	} info;
> diff --git a/drivers/peci/internal.h b/drivers/peci/internal.h
> index 9d75ea54504c..7ead53e5598f 100644
> --- a/drivers/peci/internal.h
> +++ b/drivers/peci/internal.h
> @@ -71,8 +71,7 @@ struct peci_request *peci_xfer_ep_mmio64_readl(struct
> peci_device *device, u8 ba
>   */
>  struct peci_device_id {
>  	const void *data;
> -	u16 family;
> -	u8 model;
> +	u32 x86_vfm;
>  };
>  
>  extern struct device_type peci_device_type;
> diff --git a/drivers/hwmon/peci/cputemp.c b/drivers/hwmon/peci/cputemp.c
> index a812c15948d9..5a682195b98f 100644
> --- a/drivers/hwmon/peci/cputemp.c
> +++ b/drivers/hwmon/peci/cputemp.c
> @@ -360,10 +360,10 @@ static int init_core_mask(struct peci_cputemp *priv)
>  	int ret;
>  
>  	/* Get the RESOLVED_CORES register value */
> -	switch (peci_dev->info.model) {
> -	case INTEL_FAM6_ICELAKE_X:
> -	case INTEL_FAM6_ICELAKE_D:
> -	case INTEL_FAM6_SAPPHIRERAPIDS_X:
> +	switch (peci_dev->info.x86_vfm) {
> +	case INTEL_ICELAKE_X:
> +	case INTEL_ICELAKE_D:
> +	case INTEL_SAPPHIRERAPIDS_X:
>  		ret = peci_ep_pci_local_read(peci_dev, 0, reg->bus, reg->dev,
>  					     reg->func, reg->offset + 4,
> &data);
>  		if (ret)
> diff --git a/drivers/peci/core.c b/drivers/peci/core.c
> index 0f83a9c6093b..b2d7adf05ba0 100644
> --- a/drivers/peci/core.c
> +++ b/drivers/peci/core.c
> @@ -163,9 +163,8 @@ EXPORT_SYMBOL_NS_GPL(devm_peci_controller_add, PECI);
>  static const struct peci_device_id *
>  peci_bus_match_device_id(const struct peci_device_id *id, struct peci_device
> *device)
>  {
> -	while (id->family != 0) {
> -		if (id->family == device->info.family &&
> -		    id->model == device->info.model)
> +	while (id->x86_vfm != 0) {
> +		if (id->x86_vfm == device->info.x86_vfm)
>  			return id;
>  		id++;
>  	}
> diff --git a/drivers/peci/cpu.c b/drivers/peci/cpu.c
> index bd990acd92b8..152bbd8e717a 100644
> --- a/drivers/peci/cpu.c
> +++ b/drivers/peci/cpu.c
> @@ -294,38 +294,31 @@ peci_cpu_probe(struct peci_device *device, const struct
> peci_device_id *id)
>  
>  static const struct peci_device_id peci_cpu_device_ids[] = {
>  	{ /* Haswell Xeon */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_HASWELL_X,
> +		.x86_vfm = INTEL_HASWELL_X,
>  		.data	= "hsx",
>  	},
>  	{ /* Broadwell Xeon */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_BROADWELL_X,
> +		.x86_vfm = INTEL_BROADWELL_X,
>  		.data	= "bdx",
>  	},
>  	{ /* Broadwell Xeon D */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_BROADWELL_D,
> +		.x86_vfm = INTEL_BROADWELL_D,
>  		.data	= "bdxd",
>  	},
>  	{ /* Skylake Xeon */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_SKYLAKE_X,
> +		.x86_vfm = INTEL_SKYLAKE_X,
>  		.data	= "skx",
>  	},
>  	{ /* Icelake Xeon */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_ICELAKE_X,
> +		.x86_vfm = INTEL_ICELAKE_X,
>  		.data	= "icx",
>  	},
>  	{ /* Icelake Xeon D */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_ICELAKE_D,
> +		.x86_vfm = INTEL_ICELAKE_D,
>  		.data	= "icxd",
>  	},
>  	{ /* Sapphire Rapids Xeon */
> -		.family	= 6,
> -		.model	= INTEL_FAM6_SAPPHIRERAPIDS_X,
> +		.x86_vfm = INTEL_SAPPHIRERAPIDS_X,
>  		.data	= "spr",
>  	},
>  	{ }
> diff --git a/drivers/peci/device.c b/drivers/peci/device.c
> index e6b0bffb14f4..5eb57b503c81 100644
> --- a/drivers/peci/device.c
> +++ b/drivers/peci/device.c
> @@ -100,8 +100,7 @@ static int peci_device_info_init(struct peci_device
> *device)
>  	if (ret)
>  		return ret;
>  
> -	device->info.family = peci_x86_cpu_family(cpu_id);
> -	device->info.model = peci_x86_cpu_model(cpu_id);
> +	device->info.x86_vfm = IFM(peci_x86_cpu_family(cpu_id),
> peci_x86_cpu_model(cpu_id));
>  
>  	ret = peci_get_revision(device, &revision);
>  	if (ret)