[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8690bcea-3ff4-49f1-a671-16583b8d241e@roeck-us.net>
Date: Tue, 16 Apr 2024 16:37:05 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Luck, Tony" <tony.luck@...el.com>
Cc: Jean Delvare <jdelvare@...e.com>,
"Winiarska, Iwona" <iwona.winiarska@...el.com>,
"linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: Re: [PATCH v3 50/74] x86/cpu/vfm: Update drivers/hwmon/peci/cputemp.c
On Tue, Apr 16, 2024 at 11:05:15PM +0000, Luck, Tony wrote:
> Guenter,
>
> Thanks for taking a look at this patch.
>
> > > + case VFM_MODEL(INTEL_ICELAKE_X):
> > > + case VFM_MODEL(INTEL_ICELAKE_D):
> > > + case VFM_MODEL(INTEL_SAPPHIRERAPIDS_X):
> >
> > $ git describe
> > v6.9-rc4-31-g96fca68c4fbf
> > $ git grep VFM_MODEL
> > $
> >
> > So I guess this depends on some other patch ?
> > That might be worth mentioning, especially since
> >
> > $ git describe
> > next-20240416
> > $ git grep VFM_MODEL
> > $
> >
> > doesn't find it either.
>
>
> Yes. This depends on parts 1,2,3 of this series. I should have added
> that to the "---" meta comment part of these patches that I'm spraying
> out to maintainers of random subsystems that use INTEL_FAM6 defines.
>
> > On top of that, it looks like
> >
> > #include <asm/cpu_device_id.h>
> >
> > introduces a dependency on X86 which is not currently the case.
> > CONFIG_PECI is typically used on BMCs. Many of those do not use
> > Intel CPUs. It does not seem appropriate to make support for PECI
> > based hardware monitoring dependent on it running on an Intel CPU.
>
> It seems that some use (or need to know about) Icelake and Sapphire Rapids.
>
> How does this code get the value "peci_dev->info.model" used in this switch?
>
My understanding is that the PECI protocol provides that information.
> Given that it is doing some magic just for some recent Intel CPUs, it seems
> plausible that when non-family 6 CPUs appear in the market, it might have
> to grab both the family and the model to reliably determine what to do?
>
> Simple options to avoid including <asm/cpu_device_id.h> would be
> to either:
>
> 1) provide a copy of the VFM_MODEL macro here.
>
> or
>
> 2) Keep using the old INTEL_FAM6_* #define names, but define those for
> the three CPU models peci needs locally instead of getting them from
> <asm/intel-family.h>. It looks like there is a somewhat convoluted path to
> include that. I see in <linux/peci.cpu.h>
>
> #include "../../arch/x86/include/asm/intel-family.h"
>
>
> Or maybe some better option?
>
If the CPU defines and the new macro are to be kept in architecture code,
maybe include arch/x86/include/asm/cpu_device_id.h from linux/peci.cpu.h.
That would not be worse than today's include of intel-family.h.
Guenter
Powered by blists - more mailing lists