Re: [PATCH v4 55/71] hwmon: (peci/cputemp) Switch to new Intel CPU model defines
From: Winiarska, Iwona
Date: Thu Apr 25 2024 - 21:07:49 EST
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@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 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)