Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
Hi Andy, Thanks a lot for your review. Please check my inline answers. On 4/24/2018 8:56 AM, Andy Shevchenko wrote: On Tue, 2018-04-10 at 11:32 -0700, Jae Hyun Yoo wrote: drivers/hwmon/peci-cputemp.c | 783 ++ drivers/hwmon/peci-dimmtemp.c | 432 +++ Does it make sense one driver per patch? Yes, I'll separate it into two patches. +#define CLIENT_CPU_ID_MASK0xf0ff0 /* Mask for Family / Model info */ +struct cpu_gen_info { + u32 type; + u32 cpu_id; + u32 core_max; +}; +static const struct cpu_gen_info cpu_gen_info_table[] = { + { .type = CPU_GEN_HSX, + .cpu_id = 0x306f0, /* Family code: 6, Model number: 63 (0x3f) */ + .core_max = CORE_MAX_ON_HSX }, + { .type = CPU_GEN_BRX, + .cpu_id = 0x406f0, /* Family code: 6, Model number: 79 (0x4f) */ + .core_max = CORE_MAX_ON_BDX }, + { .type = CPU_GEN_SKX, + .cpu_id = 0x50650, /* Family code: 6, Model number: 85 (0x55) */ + .core_max = CORE_MAX_ON_SKX }, +}; Are we talking about x86 CPU IDs here? If so, why x86 corresponding headers, including intel-family.h are not used? Yes, that would make more sense. I'll include the intel-family.h and will use these defines instead: INTEL_FAM6_HASWELL_X INTEL_FAM6_BROADWELL_X INTEL_FAM6_SKYLAKE_X Thanks, Jae -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On Tue, 2018-04-10 at 11:32 -0700, Jae Hyun Yoo wrote: > drivers/hwmon/peci-cputemp.c | 783 > ++ > drivers/hwmon/peci-dimmtemp.c | 432 +++ Does it make sense one driver per patch? > +#define CLIENT_CPU_ID_MASK0xf0ff0 /* Mask for Family / Model > info */ > +struct cpu_gen_info { > + u32 type; > + u32 cpu_id; > + u32 core_max; > +}; > > +static const struct cpu_gen_info cpu_gen_info_table[] = { > + { .type = CPU_GEN_HSX, > + .cpu_id = 0x306f0, /* Family code: 6, Model number: 63 > (0x3f) */ > + .core_max = CORE_MAX_ON_HSX }, > + { .type = CPU_GEN_BRX, > + .cpu_id = 0x406f0, /* Family code: 6, Model number: 79 > (0x4f) */ > + .core_max = CORE_MAX_ON_BDX }, > + { .type = CPU_GEN_SKX, > + .cpu_id = 0x50650, /* Family code: 6, Model number: 85 > (0x55) */ > + .core_max = CORE_MAX_ON_SKX }, > +}; Are we talking about x86 CPU IDs here? If so, why x86 corresponding headers, including intel-family.h are not used? -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On 4/12/2018 10:37 AM, Guenter Roeck wrote: On Thu, Apr 12, 2018 at 10:09:51AM -0700, Jae Hyun Yoo wrote: [ ... ] +static int find_core_index(struct peci_cputemp *priv, int channel) +{ + int core_channel = channel - DEFAULT_CHANNEL_NUMS; + int idx, found = 0; + + for (idx = 0; idx < priv->gen_info->core_max; idx++) { + if (priv->core_mask & BIT(idx)) { + if (core_channel == found) + break; + + found++; + } + } + + return idx; What if nothing is found ? Core temperature group will be registered only when it detects at least one core checked by check_resolved_cores(), so find_core_index() can be called only when priv->core_mask has a non-zero value. The 'nothing is found' case will not happen. That doesn't guarantee a match. If what you are saying is correct there should always be a well defined match of channel -> idx, and the search should be unnecessary. There could be some disabled cores in the resolved core mask bit sequence also it should remove indexing gap in channel numbering so it is the reason why this search function is needed. Well defined match of channel -> idx would not be always satisfied. Are you saying that each call to the function, with the same parameters, can return a different result ? No, the result will be consistent. After reading the priv->core_mask once in check_resolved_cores(), the value will not be changed. I'm saying about this case, for example if core number 2 is unresolved in total 4 cores, then the idx order will be '0, 1, 3' but channel order will be '5, 6, 7' without making any indexing gap. And you yet you claim that this is not well defined ? Or are you concerned about the amount of memory consumed by providing an array for the mapping ? Note that an indexing gap is acceptable and, in many cases, preferred. If the indexing gap is acceptable, the index search function isn't needed anymore. I'll fix all relating code to make that use direct mapping of channel -> idx then. Thanks! [ ... ] + + dev_dbg(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), priv->name); + Why does this message display the device name twice ? For an example, dev_name(hwmon_dev) shows 'hwmon5' and priv->name shows 'peci-cputemp0'. And dev_dbg() shows another device name. So you'll have something like peci-cputemp0: hwmon5: sensor 'peci-cputemp0' Practically it shows like peci-cputemp 0-30:00: hwmon10: sensor 'peci_cputemp.cpu0' where 0-30:00 is assigned by peci core. And what message would you see for cpu1 ? It shows like peci-cputemp 0-31:00: hwmon10: sensor 'peci_cputemp.cpu1' -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On Thu, Apr 12, 2018 at 10:09:51AM -0700, Jae Hyun Yoo wrote: [ ... ] > >>+static int find_core_index(struct peci_cputemp *priv, int channel) > >>+{ > >>+ int core_channel = channel - DEFAULT_CHANNEL_NUMS; > >>+ int idx, found = 0; > >>+ > >>+ for (idx = 0; idx < priv->gen_info->core_max; idx++) { > >>+ if (priv->core_mask & BIT(idx)) { > >>+ if (core_channel == found) > >>+ break; > >>+ > >>+ found++; > >>+ } > >>+ } > >>+ > >>+ return idx; > > > >What if nothing is found ? > > > > Core temperature group will be registered only when it detects at > least one core checked by check_resolved_cores(), so > find_core_index() can be called only when priv->core_mask has a > non-zero value. The 'nothing is found' case will not happen. > > >>>That doesn't guarantee a match. If what you are saying is correct > >>>there should always be > >>>a well defined match of channel -> idx, and the search should be > >>>unnecessary. > >>> > >> > >>There could be some disabled cores in the resolved core mask bit > >>sequence also it should remove indexing gap in channel numbering so it > >>is the reason why this search function is needed. Well defined match of > >>channel -> idx would not be always satisfied. > >> > >Are you saying that each call to the function, with the same parameters, > >can return a different result ? > > > > No, the result will be consistent. After reading the priv->core_mask once in > check_resolved_cores(), the value will not be changed. I'm saying about this > case, for example if core number 2 is unresolved in total 4 cores, then the > idx order will be '0, 1, 3' but channel order will be '5, 6, 7' without > making any indexing gap. > And you yet you claim that this is not well defined ? Or are you concerned about the amount of memory consumed by providing an array for the mapping ? Note that an indexing gap is acceptable and, in many cases, preferred. [ ... ] > >>+ > >>+ dev_dbg(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), > >>priv->name); > >>+ > >>> > >>>Why does this message display the device name twice ? > >>> > >> > >>For an example, dev_name(hwmon_dev) shows 'hwmon5' and priv->name shows > >>'peci-cputemp0'. > >> > >And dev_dbg() shows another device name. So you'll have something like > > > >peci-cputemp0: hwmon5: sensor 'peci-cputemp0' > > > > Practically it shows like > > peci-cputemp 0-30:00: hwmon10: sensor 'peci_cputemp.cpu0' > > where 0-30:00 is assigned by peci core. > And what message would you see for cpu1 ? -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On 4/11/2018 8:40 PM, Guenter Roeck wrote: On 04/11/2018 07:51 PM, Jae Hyun Yoo wrote: On 4/11/2018 5:34 PM, Guenter Roeck wrote: On 04/11/2018 02:59 PM, Jae Hyun Yoo wrote: Hi Guenter, Thanks a lot for sharing your time. Please see my inline answers. On 4/10/2018 3:28 PM, Guenter Roeck wrote: On Tue, Apr 10, 2018 at 11:32:11AM -0700, Jae Hyun Yoo wrote: This commit adds PECI cputemp and dimmtemp hwmon drivers. Signed-off-by: Jae Hyun Yoo Reviewed-by: Haiyue Wang Reviewed-by: James Feist Reviewed-by: Vernon Mauery Cc: Alan Cox Cc: Andrew Jeffery Cc: Andrew Lunn Cc: Andy Shevchenko Cc: Arnd Bergmann Cc: Benjamin Herrenschmidt Cc: Fengguang Wu Cc: Greg KH Cc: Guenter Roeck Cc: Jason M Biils Cc: Jean Delvare Cc: Joel Stanley Cc: Julia Cartwright Cc: Miguel Ojeda Cc: Milton Miller II Cc: Pavel Machek Cc: Randy Dunlap Cc: Stef van Os Cc: Sumeet R Pawnikar --- drivers/hwmon/Kconfig | 28 ++ drivers/hwmon/Makefile | 2 + drivers/hwmon/peci-cputemp.c | 783 ++ drivers/hwmon/peci-dimmtemp.c | 432 +++ 4 files changed, 1245 insertions(+) create mode 100644 drivers/hwmon/peci-cputemp.c create mode 100644 drivers/hwmon/peci-dimmtemp.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index f249a4428458..c52f610f81d0 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1259,6 +1259,34 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_PECI_CPUTEMP + tristate "PECI CPU temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI + cputemp driver which provides Digital Thermal Sensor (DTS) thermal + readings of the CPU package and CPU cores that are accessible using + the PECI Client Command Suite via the processor PECI client. + Check Documentation/hwmon/peci-cputemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-cputemp. + +config SENSORS_PECI_DIMMTEMP + tristate "PECI DIMM temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI hwmon + driver which provides Digital Thermal Sensor (DTS) thermal readings of + DIMM components that are accessible using the PECI Client Command + Suite via the processor PECI client. + Check Documentation/hwmon/peci-dimmtemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-dimmtemp. + config SENSORS_NSA320 tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" depends on GPIOLIB && OF diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e7d52a36e6c4..48d9598fcd3a 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -136,6 +136,8 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_PECI_CPUTEMP) += peci-cputemp.o +obj-$(CONFIG_SENSORS_PECI_DIMMTEMP) += peci-dimmtemp.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o diff --git a/drivers/hwmon/peci-cputemp.c b/drivers/hwmon/peci-cputemp.c new file mode 100644 index ..f0bc92687512 --- /dev/null +++ b/drivers/hwmon/peci-cputemp.c @@ -0,0 +1,783 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2018 Intel Corporation + +#include +#include +#include Is this include needed ? No it isn't. Will drop the line. +#include +#include +#include +#include + +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */ + +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */ +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */ +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */ + +#define DEFAULT_CHANNEL_NUMS 5 +#define CORETEMP_CHANNEL_NUMS CORE_MAX_ON_SKX +#define CPUTEMP_CHANNEL_NUMS (DEFAULT_CHANNEL_NUMS + CORETEMP_CHANNEL_NUMS) + +#define CLIENT_CPU_ID_MASK 0xf0ff0 /* Mask for Family / Model info */ + +#define UPDATE_INTERVAL_MIN HZ + +enum cpu_gens { + CPU_GEN_HSX, /* Haswell Xeon */ + CPU_GEN_BRX, /* Broadwell Xeon */ + CPU_GEN_SKX, /* Skylake Xeon */ + CPU_GEN_MAX +}; + +struct cpu_gen_info { + u32 type; + u32 cpu_id; + u32 core_max; +}; + +struct temp_data { + bool valid; + s32 value; + unsigned long last_updated; +}; + +struct temp_group { + struct temp_data die; + struct temp_data dts_margin; + struct temp_data tcontrol; + struct temp_data tthrottle; + struct t
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On 04/11/2018 07:51 PM, Jae Hyun Yoo wrote: On 4/11/2018 5:34 PM, Guenter Roeck wrote: On 04/11/2018 02:59 PM, Jae Hyun Yoo wrote: Hi Guenter, Thanks a lot for sharing your time. Please see my inline answers. On 4/10/2018 3:28 PM, Guenter Roeck wrote: On Tue, Apr 10, 2018 at 11:32:11AM -0700, Jae Hyun Yoo wrote: This commit adds PECI cputemp and dimmtemp hwmon drivers. Signed-off-by: Jae Hyun Yoo Reviewed-by: Haiyue Wang Reviewed-by: James Feist Reviewed-by: Vernon Mauery Cc: Alan Cox Cc: Andrew Jeffery Cc: Andrew Lunn Cc: Andy Shevchenko Cc: Arnd Bergmann Cc: Benjamin Herrenschmidt Cc: Fengguang Wu Cc: Greg KH Cc: Guenter Roeck Cc: Jason M Biils Cc: Jean Delvare Cc: Joel Stanley Cc: Julia Cartwright Cc: Miguel Ojeda Cc: Milton Miller II Cc: Pavel Machek Cc: Randy Dunlap Cc: Stef van Os Cc: Sumeet R Pawnikar --- drivers/hwmon/Kconfig | 28 ++ drivers/hwmon/Makefile | 2 + drivers/hwmon/peci-cputemp.c | 783 ++ drivers/hwmon/peci-dimmtemp.c | 432 +++ 4 files changed, 1245 insertions(+) create mode 100644 drivers/hwmon/peci-cputemp.c create mode 100644 drivers/hwmon/peci-dimmtemp.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index f249a4428458..c52f610f81d0 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1259,6 +1259,34 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_PECI_CPUTEMP + tristate "PECI CPU temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI + cputemp driver which provides Digital Thermal Sensor (DTS) thermal + readings of the CPU package and CPU cores that are accessible using + the PECI Client Command Suite via the processor PECI client. + Check Documentation/hwmon/peci-cputemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-cputemp. + +config SENSORS_PECI_DIMMTEMP + tristate "PECI DIMM temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI hwmon + driver which provides Digital Thermal Sensor (DTS) thermal readings of + DIMM components that are accessible using the PECI Client Command + Suite via the processor PECI client. + Check Documentation/hwmon/peci-dimmtemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-dimmtemp. + config SENSORS_NSA320 tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" depends on GPIOLIB && OF diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e7d52a36e6c4..48d9598fcd3a 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -136,6 +136,8 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_PECI_CPUTEMP) += peci-cputemp.o +obj-$(CONFIG_SENSORS_PECI_DIMMTEMP) += peci-dimmtemp.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o diff --git a/drivers/hwmon/peci-cputemp.c b/drivers/hwmon/peci-cputemp.c new file mode 100644 index ..f0bc92687512 --- /dev/null +++ b/drivers/hwmon/peci-cputemp.c @@ -0,0 +1,783 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2018 Intel Corporation + +#include +#include +#include Is this include needed ? No it isn't. Will drop the line. +#include +#include +#include +#include + +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */ + +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */ +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */ +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */ + +#define DEFAULT_CHANNEL_NUMS 5 +#define CORETEMP_CHANNEL_NUMS CORE_MAX_ON_SKX +#define CPUTEMP_CHANNEL_NUMS (DEFAULT_CHANNEL_NUMS + CORETEMP_CHANNEL_NUMS) + +#define CLIENT_CPU_ID_MASK 0xf0ff0 /* Mask for Family / Model info */ + +#define UPDATE_INTERVAL_MIN HZ + +enum cpu_gens { + CPU_GEN_HSX, /* Haswell Xeon */ + CPU_GEN_BRX, /* Broadwell Xeon */ + CPU_GEN_SKX, /* Skylake Xeon */ + CPU_GEN_MAX +}; + +struct cpu_gen_info { + u32 type; + u32 cpu_id; + u32 core_max; +}; + +struct temp_data { + bool valid; + s32 value; + unsigned long last_updated; +}; + +struct temp_group { + struct temp_data die; + struct temp_data dts_margin; + struct temp_data tcontrol; + struct temp_data tthrottle; + struct temp_data tjmax; + struct temp_data core[CORETEMP_CHANNEL_NUMS]
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On 4/11/2018 5:34 PM, Guenter Roeck wrote: On 04/11/2018 02:59 PM, Jae Hyun Yoo wrote: Hi Guenter, Thanks a lot for sharing your time. Please see my inline answers. On 4/10/2018 3:28 PM, Guenter Roeck wrote: On Tue, Apr 10, 2018 at 11:32:11AM -0700, Jae Hyun Yoo wrote: This commit adds PECI cputemp and dimmtemp hwmon drivers. Signed-off-by: Jae Hyun Yoo Reviewed-by: Haiyue Wang Reviewed-by: James Feist Reviewed-by: Vernon Mauery Cc: Alan Cox Cc: Andrew Jeffery Cc: Andrew Lunn Cc: Andy Shevchenko Cc: Arnd Bergmann Cc: Benjamin Herrenschmidt Cc: Fengguang Wu Cc: Greg KH Cc: Guenter Roeck Cc: Jason M Biils Cc: Jean Delvare Cc: Joel Stanley Cc: Julia Cartwright Cc: Miguel Ojeda Cc: Milton Miller II Cc: Pavel Machek Cc: Randy Dunlap Cc: Stef van Os Cc: Sumeet R Pawnikar --- drivers/hwmon/Kconfig | 28 ++ drivers/hwmon/Makefile | 2 + drivers/hwmon/peci-cputemp.c | 783 ++ drivers/hwmon/peci-dimmtemp.c | 432 +++ 4 files changed, 1245 insertions(+) create mode 100644 drivers/hwmon/peci-cputemp.c create mode 100644 drivers/hwmon/peci-dimmtemp.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index f249a4428458..c52f610f81d0 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1259,6 +1259,34 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_PECI_CPUTEMP + tristate "PECI CPU temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI + cputemp driver which provides Digital Thermal Sensor (DTS) thermal + readings of the CPU package and CPU cores that are accessible using + the PECI Client Command Suite via the processor PECI client. + Check Documentation/hwmon/peci-cputemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-cputemp. + +config SENSORS_PECI_DIMMTEMP + tristate "PECI DIMM temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI hwmon + driver which provides Digital Thermal Sensor (DTS) thermal readings of + DIMM components that are accessible using the PECI Client Command + Suite via the processor PECI client. + Check Documentation/hwmon/peci-dimmtemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-dimmtemp. + config SENSORS_NSA320 tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" depends on GPIOLIB && OF diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e7d52a36e6c4..48d9598fcd3a 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -136,6 +136,8 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_PECI_CPUTEMP) += peci-cputemp.o +obj-$(CONFIG_SENSORS_PECI_DIMMTEMP) += peci-dimmtemp.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o diff --git a/drivers/hwmon/peci-cputemp.c b/drivers/hwmon/peci-cputemp.c new file mode 100644 index ..f0bc92687512 --- /dev/null +++ b/drivers/hwmon/peci-cputemp.c @@ -0,0 +1,783 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2018 Intel Corporation + +#include +#include +#include Is this include needed ? No it isn't. Will drop the line. +#include +#include +#include +#include + +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */ + +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */ +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */ +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */ + +#define DEFAULT_CHANNEL_NUMS 5 +#define CORETEMP_CHANNEL_NUMS CORE_MAX_ON_SKX +#define CPUTEMP_CHANNEL_NUMS (DEFAULT_CHANNEL_NUMS + CORETEMP_CHANNEL_NUMS) + +#define CLIENT_CPU_ID_MASK 0xf0ff0 /* Mask for Family / Model info */ + +#define UPDATE_INTERVAL_MIN HZ + +enum cpu_gens { + CPU_GEN_HSX, /* Haswell Xeon */ + CPU_GEN_BRX, /* Broadwell Xeon */ + CPU_GEN_SKX, /* Skylake Xeon */ + CPU_GEN_MAX +}; + +struct cpu_gen_info { + u32 type; + u32 cpu_id; + u32 core_max; +}; + +struct temp_data { + bool valid; + s32 value; + unsigned long last_updated; +}; + +struct temp_group { + struct temp_data die; + struct temp_data dts_margin; + struct temp_data tcontrol; + struct temp_data tthrottle; + struct temp_data tjmax; + struct temp_data core[CORETEMP_CHANNEL_NUMS]; +}; + +struct peci_cpute
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On 04/11/2018 02:59 PM, Jae Hyun Yoo wrote: Hi Guenter, Thanks a lot for sharing your time. Please see my inline answers. On 4/10/2018 3:28 PM, Guenter Roeck wrote: On Tue, Apr 10, 2018 at 11:32:11AM -0700, Jae Hyun Yoo wrote: This commit adds PECI cputemp and dimmtemp hwmon drivers. Signed-off-by: Jae Hyun Yoo Reviewed-by: Haiyue Wang Reviewed-by: James Feist Reviewed-by: Vernon Mauery Cc: Alan Cox Cc: Andrew Jeffery Cc: Andrew Lunn Cc: Andy Shevchenko Cc: Arnd Bergmann Cc: Benjamin Herrenschmidt Cc: Fengguang Wu Cc: Greg KH Cc: Guenter Roeck Cc: Jason M Biils Cc: Jean Delvare Cc: Joel Stanley Cc: Julia Cartwright Cc: Miguel Ojeda Cc: Milton Miller II Cc: Pavel Machek Cc: Randy Dunlap Cc: Stef van Os Cc: Sumeet R Pawnikar --- drivers/hwmon/Kconfig | 28 ++ drivers/hwmon/Makefile | 2 + drivers/hwmon/peci-cputemp.c | 783 ++ drivers/hwmon/peci-dimmtemp.c | 432 +++ 4 files changed, 1245 insertions(+) create mode 100644 drivers/hwmon/peci-cputemp.c create mode 100644 drivers/hwmon/peci-dimmtemp.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index f249a4428458..c52f610f81d0 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1259,6 +1259,34 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_PECI_CPUTEMP + tristate "PECI CPU temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI + cputemp driver which provides Digital Thermal Sensor (DTS) thermal + readings of the CPU package and CPU cores that are accessible using + the PECI Client Command Suite via the processor PECI client. + Check Documentation/hwmon/peci-cputemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-cputemp. + +config SENSORS_PECI_DIMMTEMP + tristate "PECI DIMM temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI hwmon + driver which provides Digital Thermal Sensor (DTS) thermal readings of + DIMM components that are accessible using the PECI Client Command + Suite via the processor PECI client. + Check Documentation/hwmon/peci-dimmtemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-dimmtemp. + config SENSORS_NSA320 tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" depends on GPIOLIB && OF diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e7d52a36e6c4..48d9598fcd3a 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -136,6 +136,8 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_PECI_CPUTEMP) += peci-cputemp.o +obj-$(CONFIG_SENSORS_PECI_DIMMTEMP) += peci-dimmtemp.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o diff --git a/drivers/hwmon/peci-cputemp.c b/drivers/hwmon/peci-cputemp.c new file mode 100644 index ..f0bc92687512 --- /dev/null +++ b/drivers/hwmon/peci-cputemp.c @@ -0,0 +1,783 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2018 Intel Corporation + +#include +#include +#include Is this include needed ? No it isn't. Will drop the line. +#include +#include +#include +#include + +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */ + +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */ +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */ +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */ + +#define DEFAULT_CHANNEL_NUMS 5 +#define CORETEMP_CHANNEL_NUMS CORE_MAX_ON_SKX +#define CPUTEMP_CHANNEL_NUMS (DEFAULT_CHANNEL_NUMS + CORETEMP_CHANNEL_NUMS) + +#define CLIENT_CPU_ID_MASK 0xf0ff0 /* Mask for Family / Model info */ + +#define UPDATE_INTERVAL_MIN HZ + +enum cpu_gens { + CPU_GEN_HSX, /* Haswell Xeon */ + CPU_GEN_BRX, /* Broadwell Xeon */ + CPU_GEN_SKX, /* Skylake Xeon */ + CPU_GEN_MAX +}; + +struct cpu_gen_info { + u32 type; + u32 cpu_id; + u32 core_max; +}; + +struct temp_data { + bool valid; + s32 value; + unsigned long last_updated; +}; + +struct temp_group { + struct temp_data die; + struct temp_data dts_margin; + struct temp_data tcontrol; + struct temp_data tthrottle; + struct temp_data tjmax; + struct temp_data core[CORETEMP_CHANNEL_NUMS]; +}; + +struct peci_cputemp { + struct peci_client *client; + struct device *dev;
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
Hi Guenter, Thanks a lot for sharing your time. Please see my inline answers. On 4/10/2018 3:28 PM, Guenter Roeck wrote: On Tue, Apr 10, 2018 at 11:32:11AM -0700, Jae Hyun Yoo wrote: This commit adds PECI cputemp and dimmtemp hwmon drivers. Signed-off-by: Jae Hyun Yoo Reviewed-by: Haiyue Wang Reviewed-by: James Feist Reviewed-by: Vernon Mauery Cc: Alan Cox Cc: Andrew Jeffery Cc: Andrew Lunn Cc: Andy Shevchenko Cc: Arnd Bergmann Cc: Benjamin Herrenschmidt Cc: Fengguang Wu Cc: Greg KH Cc: Guenter Roeck Cc: Jason M Biils Cc: Jean Delvare Cc: Joel Stanley Cc: Julia Cartwright Cc: Miguel Ojeda Cc: Milton Miller II Cc: Pavel Machek Cc: Randy Dunlap Cc: Stef van Os Cc: Sumeet R Pawnikar --- drivers/hwmon/Kconfig | 28 ++ drivers/hwmon/Makefile| 2 + drivers/hwmon/peci-cputemp.c | 783 ++ drivers/hwmon/peci-dimmtemp.c | 432 +++ 4 files changed, 1245 insertions(+) create mode 100644 drivers/hwmon/peci-cputemp.c create mode 100644 drivers/hwmon/peci-dimmtemp.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index f249a4428458..c52f610f81d0 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1259,6 +1259,34 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_PECI_CPUTEMP + tristate "PECI CPU temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI + cputemp driver which provides Digital Thermal Sensor (DTS) thermal + readings of the CPU package and CPU cores that are accessible using + the PECI Client Command Suite via the processor PECI client. + Check Documentation/hwmon/peci-cputemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-cputemp. + +config SENSORS_PECI_DIMMTEMP + tristate "PECI DIMM temperature monitoring support" + depends on OF + depends on PECI + help + If you say yes here you get support for the generic Intel PECI hwmon + driver which provides Digital Thermal Sensor (DTS) thermal readings of + DIMM components that are accessible using the PECI Client Command + Suite via the processor PECI client. + Check Documentation/hwmon/peci-dimmtemp for details. + + This driver can also be built as a module. If so, the module + will be called peci-dimmtemp. + config SENSORS_NSA320 tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" depends on GPIOLIB && OF diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e7d52a36e6c4..48d9598fcd3a 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -136,6 +136,8 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_PECI_CPUTEMP) += peci-cputemp.o +obj-$(CONFIG_SENSORS_PECI_DIMMTEMP)+= peci-dimmtemp.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o diff --git a/drivers/hwmon/peci-cputemp.c b/drivers/hwmon/peci-cputemp.c new file mode 100644 index ..f0bc92687512 --- /dev/null +++ b/drivers/hwmon/peci-cputemp.c @@ -0,0 +1,783 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2018 Intel Corporation + +#include +#include +#include Is this include needed ? No it isn't. Will drop the line. +#include +#include +#include +#include + +#define TEMP_TYPE_PECI6 /* Sensor type 6: Intel PECI */ + +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */ +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */ +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */ + +#define DEFAULT_CHANNEL_NUMS 5 +#define CORETEMP_CHANNEL_NUMS CORE_MAX_ON_SKX +#define CPUTEMP_CHANNEL_NUMS (DEFAULT_CHANNEL_NUMS + CORETEMP_CHANNEL_NUMS) + +#define CLIENT_CPU_ID_MASK0xf0ff0 /* Mask for Family / Model info */ + +#define UPDATE_INTERVAL_MIN HZ + +enum cpu_gens { + CPU_GEN_HSX, /* Haswell Xeon */ + CPU_GEN_BRX, /* Broadwell Xeon */ + CPU_GEN_SKX, /* Skylake Xeon */ + CPU_GEN_MAX +}; + +struct cpu_gen_info { + u32 type; + u32 cpu_id; + u32 core_max; +}; + +struct temp_data { + bool valid; + s32 value; + unsigned long last_updated; +}; + +struct temp_group { + struct temp_data die; + struct temp_data dts_margin; + struct temp_data tcontrol; + struct temp_data tthrottle; + struct temp_data tjmax; + struct temp_data core[CORETEMP_CHANNEL_NUMS]; +}; + +struct peci
Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers
On Tue, Apr 10, 2018 at 11:32:11AM -0700, Jae Hyun Yoo wrote: > This commit adds PECI cputemp and dimmtemp hwmon drivers. > > Signed-off-by: Jae Hyun Yoo > Reviewed-by: Haiyue Wang > Reviewed-by: James Feist > Reviewed-by: Vernon Mauery > Cc: Alan Cox > Cc: Andrew Jeffery > Cc: Andrew Lunn > Cc: Andy Shevchenko > Cc: Arnd Bergmann > Cc: Benjamin Herrenschmidt > Cc: Fengguang Wu > Cc: Greg KH > Cc: Guenter Roeck > Cc: Jason M Biils > Cc: Jean Delvare > Cc: Joel Stanley > Cc: Julia Cartwright > Cc: Miguel Ojeda > Cc: Milton Miller II > Cc: Pavel Machek > Cc: Randy Dunlap > Cc: Stef van Os > Cc: Sumeet R Pawnikar > --- > drivers/hwmon/Kconfig | 28 ++ > drivers/hwmon/Makefile| 2 + > drivers/hwmon/peci-cputemp.c | 783 > ++ > drivers/hwmon/peci-dimmtemp.c | 432 +++ > 4 files changed, 1245 insertions(+) > create mode 100644 drivers/hwmon/peci-cputemp.c > create mode 100644 drivers/hwmon/peci-dimmtemp.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index f249a4428458..c52f610f81d0 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1259,6 +1259,34 @@ config SENSORS_NCT7904 > This driver can also be built as a module. If so, the module > will be called nct7904. > > +config SENSORS_PECI_CPUTEMP > + tristate "PECI CPU temperature monitoring support" > + depends on OF > + depends on PECI > + help > + If you say yes here you get support for the generic Intel PECI > + cputemp driver which provides Digital Thermal Sensor (DTS) thermal > + readings of the CPU package and CPU cores that are accessible using > + the PECI Client Command Suite via the processor PECI client. > + Check Documentation/hwmon/peci-cputemp for details. > + > + This driver can also be built as a module. If so, the module > + will be called peci-cputemp. > + > +config SENSORS_PECI_DIMMTEMP > + tristate "PECI DIMM temperature monitoring support" > + depends on OF > + depends on PECI > + help > + If you say yes here you get support for the generic Intel PECI hwmon > + driver which provides Digital Thermal Sensor (DTS) thermal readings of > + DIMM components that are accessible using the PECI Client Command > + Suite via the processor PECI client. > + Check Documentation/hwmon/peci-dimmtemp for details. > + > + This driver can also be built as a module. If so, the module > + will be called peci-dimmtemp. > + > config SENSORS_NSA320 > tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" > depends on GPIOLIB && OF > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index e7d52a36e6c4..48d9598fcd3a 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -136,6 +136,8 @@ obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o > obj-$(CONFIG_SENSORS_NCT7904)+= nct7904.o > obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o > obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o > +obj-$(CONFIG_SENSORS_PECI_CPUTEMP) += peci-cputemp.o > +obj-$(CONFIG_SENSORS_PECI_DIMMTEMP) += peci-dimmtemp.o > obj-$(CONFIG_SENSORS_PC87360)+= pc87360.o > obj-$(CONFIG_SENSORS_PC87427)+= pc87427.o > obj-$(CONFIG_SENSORS_PCF8591)+= pcf8591.o > diff --git a/drivers/hwmon/peci-cputemp.c b/drivers/hwmon/peci-cputemp.c > new file mode 100644 > index ..f0bc92687512 > --- /dev/null > +++ b/drivers/hwmon/peci-cputemp.c > @@ -0,0 +1,783 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2018 Intel Corporation > + > +#include > +#include > +#include Is this include needed ? > +#include > +#include > +#include > +#include > + > +#define TEMP_TYPE_PECI6 /* Sensor type 6: Intel PECI */ > + > +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */ > +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */ > +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */ > + > +#define DEFAULT_CHANNEL_NUMS 5 > +#define CORETEMP_CHANNEL_NUMS CORE_MAX_ON_SKX > +#define CPUTEMP_CHANNEL_NUMS (DEFAULT_CHANNEL_NUMS + CORETEMP_CHANNEL_NUMS) > + > +#define CLIENT_CPU_ID_MASK0xf0ff0 /* Mask for Family / Model info */ > + > +#define UPDATE_INTERVAL_MIN HZ > + > +enum cpu_gens { > + CPU_GEN_HSX, /* Haswell Xeon */ > + CPU_GEN_BRX, /* Broadwell Xeon */ > + CPU_GEN_SKX, /* Skylake Xeon */ > + CPU_GEN_MAX > +}; > + > +struct cpu_gen_info { > + u32 type; > + u32 cpu_id; > + u32 core_max; > +}; > + > +struct temp_data { > + bool valid; > + s32 value; > + unsigned long last_updated; > +}; > + > +struct temp_group { > + struct temp_data die; > + struct temp_data dts_margin; > + struct temp_data tcontrol; > + struct temp_data tthrottle; > + struct temp_data tjmax; > + struct temp_data co