Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Jae Hyun Yoo]

Hi Stef,

Thanks for sharing your time to test it.

That is expected result in v2. Previously in v1, it used delayed 
creation on core temperature group so it was okay if hwmon driver is 
registered when client CPU is powered down, but in v2, the driver should 
check resolved cores at probing time to prevent the delayed creation on 
core temperature group and indexing gap which breaks hwmon subsystem's 
common rule, so I added peci_detect() into peci_new_device() in PECI 
core driver to check online status of the client CPU when registering a 
new device.


You may need to use dynamic dtoverlay for loading/unloading a PECI hwmon 
driver according to the current client CPU power state. It means a PECI 
hwmon driver can be registered only when the client CPU is powered on. 
This design will be kept in v3 as well.


Thanks,
Jae

On 3/13/2018 2:32 AM, Stef van Os wrote:

Hi Jae,

I tried version 1 and 2 of your PECI patch on our (AST2500 / Xeon E5 v4) 
system. The V1 patchset works as expected (reading back temperature 0 
until PECI is up), but the hwmon driver probe fails with version 2. It 
communicates with the Xeon and assumes during kernel boot of the Aspeed 
that PECI to the Xeon's is already up and running, but our system 
enables the main Xeon supplies from AST2500 userspace.


If I load the hwmon driver as a module to load later on, the driver does 
not call probe like e.g. a I2C driver on the I2C bus does. Am I using V2 
wrongly?


BR,
Stef

On 02/21/2018 05:16 PM, Jae Hyun Yoo wrote:

This commit adds a generic PECI hwmon client driver implementation.

Signed-off-by: Jae Hyun Yoo 
---
  drivers/hwmon/Kconfig  |  10 +
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/peci-hwmon.c | 928 
+

  3 files changed, 939 insertions(+)
  create mode 100644 drivers/hwmon/peci-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553ff5cb..f22e0c31f597 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
    This driver can also be built as a module.  If so, the module
    will be called nct7904.
+config SENSORS_PECI_HWMON
+    tristate "PECI hwmon support"
+    depends on PECI
+    help
+  If you say yes here you get support for the generic PECI hwmon
+  driver.
+
+  This driver can also be built as a module.  If so, the module
+  will be called peci-hwmon.
+
  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 f814b4ace138..946f54b168e5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -135,6 +135,7 @@ 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_HWMON)    += peci-hwmon.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-hwmon.c b/drivers/hwmon/peci-hwmon.c
new file mode 100644
index ..edd27744adcb
--- /dev/null
+++ b/drivers/hwmon/peci-hwmon.c
@@ -0,0 +1,928 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Intel Corporation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DIMM_SLOT_NUMS_MAX    12  /* Max DIMM numbers (channel ranks 
x 2) */
+#define CORE_NUMS_MAX 28  /* Max core numbers (max on SKX 
Platinum) */

+#define TEMP_TYPE_PECI    6   /* Sensor type 6: Intel PECI */
+
+#define CORE_TEMP_ATTRS   5
+#define DIMM_TEMP_ATTRS   2
+#define ATTR_NAME_LEN 24
+
+#define DEFAULT_ATTR_GRP_NUMS 5
+
+#define UPDATE_INTERVAL_MIN   HZ
+#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
+
+enum sign {
+    POS,
+    NEG
+};
+
+struct temp_data {
+    bool valid;
+    s32  value;
+    unsigned long last_updated;
+};
+
+struct temp_group {
+    struct temp_data tjmax;
+    struct temp_data tcontrol;
+    struct temp_data tthrottle;
+    struct temp_data dts_margin;
+    struct temp_data die;
+    struct temp_data core[CORE_NUMS_MAX];
+    struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
+};
+
+struct core_temp_group {
+    struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
+    char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
+    struct attribute *attrs[CORE_TEMP_ATTRS + 1];
+    struct attribute_group attr_group;
+};
+
+struct dimm_temp_group {
+    struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
+    char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
+    struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
+    struct attribute_group attr_group;
+};
+
+struct peci_hwmon {
+    struct peci_client *client;
+    struct device *dev;
+    struct device 

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Stef van Os]

Hi Jae,

I tried version 1 and 2 of your PECI patch on our (AST2500 / Xeon E5 v4) 
system. The V1 patchset works as expected (reading back temperature 0 
until PECI is up), but the hwmon driver probe fails with version 2. It 
communicates with the Xeon and assumes during kernel boot of the Aspeed 
that PECI to the Xeon's is already up and running, but our system 
enables the main Xeon supplies from AST2500 userspace.


If I load the hwmon driver as a module to load later on, the driver does 
not call probe like e.g. a I2C driver on the I2C bus does. Am I using V2 
wrongly?


BR,
Stef

On 02/21/2018 05:16 PM, Jae Hyun Yoo wrote:

This commit adds a generic PECI hwmon client driver implementation.

Signed-off-by: Jae Hyun Yoo 
---
  drivers/hwmon/Kconfig  |  10 +
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/peci-hwmon.c | 928 +
  3 files changed, 939 insertions(+)
  create mode 100644 drivers/hwmon/peci-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553ff5cb..f22e0c31f597 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
  This driver can also be built as a module.  If so, the module
  will be called nct7904.
  
+config SENSORS_PECI_HWMON

+   tristate "PECI hwmon support"
+   depends on PECI
+   help
+ If you say yes here you get support for the generic PECI hwmon
+ driver.
+
+ This driver can also be built as a module.  If so, the module
+ will be called peci-hwmon.
+
  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 f814b4ace138..946f54b168e5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -135,6 +135,7 @@ 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_HWMON)   += peci-hwmon.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-hwmon.c b/drivers/hwmon/peci-hwmon.c
new file mode 100644
index ..edd27744adcb
--- /dev/null
+++ b/drivers/hwmon/peci-hwmon.c
@@ -0,0 +1,928 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Intel Corporation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DIMM_SLOT_NUMS_MAX12  /* Max DIMM numbers (channel ranks x 2) */
+#define CORE_NUMS_MAX 28  /* Max core numbers (max on SKX Platinum) */
+#define TEMP_TYPE_PECI6   /* Sensor type 6: Intel PECI */
+
+#define CORE_TEMP_ATTRS   5
+#define DIMM_TEMP_ATTRS   2
+#define ATTR_NAME_LEN 24
+
+#define DEFAULT_ATTR_GRP_NUMS 5
+
+#define UPDATE_INTERVAL_MIN   HZ
+#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
+
+enum sign {
+   POS,
+   NEG
+};
+
+struct temp_data {
+   bool valid;
+   s32  value;
+   unsigned long last_updated;
+};
+
+struct temp_group {
+   struct temp_data tjmax;
+   struct temp_data tcontrol;
+   struct temp_data tthrottle;
+   struct temp_data dts_margin;
+   struct temp_data die;
+   struct temp_data core[CORE_NUMS_MAX];
+   struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
+};
+
+struct core_temp_group {
+   struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
+   char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
+   struct attribute *attrs[CORE_TEMP_ATTRS + 1];
+   struct attribute_group attr_group;
+};
+
+struct dimm_temp_group {
+   struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
+   char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
+   struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
+   struct attribute_group attr_group;
+};
+
+struct peci_hwmon {
+   struct peci_client *client;
+   struct device *dev;
+   struct device *hwmon_dev;
+   struct workqueue_struct *work_queue;
+   struct delayed_work work_handler;
+   char name[PECI_NAME_SIZE];
+   struct temp_group temp;
+   u8 addr;
+   uint cpu_no;
+   u32 core_mask;
+   u32 dimm_mask;
+   const struct attribute_group *core_attr_groups[CORE_NUMS_MAX + 1];
+   const struct attribute_group *dimm_attr_groups[DIMM_SLOT_NUMS_MAX + 1];
+   uint global_idx;
+   uint core_idx;
+   uint dimm_idx;
+};
+
+enum label {
+   L_DIE,
+   L_DTS,
+   L_TCONTROL,
+   L_TTHROTTLE,
+   L_TJMAX,
+   L_MAX
+};
+
+static const char *peci_label[L_MAX] = {
+   "Die\n",
+   "DTS margin to Tcontrol\n",
+   "Tcontrol\n",
+   "Tthrottle\n",
+   "Tjmax\n",
+};
+
+static int 

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Jae Hyun Yoo]

On 2/23/2018 4:00 PM, Miguel Ojeda wrote:

On Thu, Feb 22, 2018 at 2:29 AM, Jae Hyun Yoo
 wrote:

On 2/21/2018 4:37 PM, Andrew Lunn wrote:


But even with this change, it still needs to use delayed creation
because BMC side kernel doesn't know how many DIMMs are populated on
a remote server before the remote server completes its memory
training and testing in BIOS, but it needs to check the remote
server's CPU temperature as immediate as possible to make
appropriate thermal control based on the remote CPU's temperature to
avoid any critical thermal issue. What would be a better solution in
this case?



You could change this driver so that it supports one DIMM.  Move the
'hotplug' part into another driver which creates and destroys
instances of the hwmon DIMM device as the DIMMS come and go.

Also, do you need to handle CPU hotplug? You could split the CPU
temperature part into a separate hwmon driver? And again create and
destroy devices as CPUs come and go?

 Andrew



That seems like a possible option. I'll rewrite the hwmon driver again like
that.

Thanks for the good idea. :)


By the way, in the rewrite, please try to avoid the create*workqueue()
functions (they are deprecated :).

Cheers,
Miguel



Hi Miguel,

Thanks for letting me know that. I'll replace that with 
alloc_workqueue(). :)


Regards,
Jae



Jae

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Miguel Ojeda]

On Thu, Feb 22, 2018 at 2:29 AM, Jae Hyun Yoo
 wrote:
> On 2/21/2018 4:37 PM, Andrew Lunn wrote:
>>>
>>> But even with this change, it still needs to use delayed creation
>>> because BMC side kernel doesn't know how many DIMMs are populated on
>>> a remote server before the remote server completes its memory
>>> training and testing in BIOS, but it needs to check the remote
>>> server's CPU temperature as immediate as possible to make
>>> appropriate thermal control based on the remote CPU's temperature to
>>> avoid any critical thermal issue. What would be a better solution in
>>> this case?
>>
>>
>> You could change this driver so that it supports one DIMM.  Move the
>> 'hotplug' part into another driver which creates and destroys
>> instances of the hwmon DIMM device as the DIMMS come and go.
>>
>> Also, do you need to handle CPU hotplug? You could split the CPU
>> temperature part into a separate hwmon driver? And again create and
>> destroy devices as CPUs come and go?
>>
>> Andrew
>>
>
> That seems like a possible option. I'll rewrite the hwmon driver again like
> that.
>
> Thanks for the good idea. :)

By the way, in the rewrite, please try to avoid the create*workqueue()
functions (they are deprecated :).

Cheers,
Miguel

>
> Jae
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Jae Hyun Yoo]

On 2/21/2018 4:37 PM, Andrew Lunn wrote:

But even with this change, it still needs to use delayed creation
because BMC side kernel doesn't know how many DIMMs are populated on
a remote server before the remote server completes its memory
training and testing in BIOS, but it needs to check the remote
server's CPU temperature as immediate as possible to make
appropriate thermal control based on the remote CPU's temperature to
avoid any critical thermal issue. What would be a better solution in
this case?


You could change this driver so that it supports one DIMM.  Move the
'hotplug' part into another driver which creates and destroys
instances of the hwmon DIMM device as the DIMMS come and go.

Also, do you need to handle CPU hotplug? You could split the CPU
temperature part into a separate hwmon driver? And again create and
destroy devices as CPUs come and go?

Andrew



That seems like a possible option. I'll rewrite the hwmon driver again 
like that.


Thanks for the good idea. :)

Jae
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Andrew Lunn]

> But even with this change, it still needs to use delayed creation
> because BMC side kernel doesn't know how many DIMMs are populated on
> a remote server before the remote server completes its memory
> training and testing in BIOS, but it needs to check the remote
> server's CPU temperature as immediate as possible to make
> appropriate thermal control based on the remote CPU's temperature to
> avoid any critical thermal issue. What would be a better solution in
> this case?

You could change this driver so that it supports one DIMM.  Move the
'hotplug' part into another driver which creates and destroys
instances of the hwmon DIMM device as the DIMMS come and go.

Also, do you need to handle CPU hotplug? You could split the CPU
temperature part into a separate hwmon driver? And again create and
destroy devices as CPUs come and go?

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Guenter Roeck]

On Wed, Feb 21, 2018 at 01:24:48PM -0800, Jae Hyun Yoo wrote:
> Hi Guenter,
> 
> Thanks for sharing your time to review this code. Please check my answers
> inline.
> 
> On 2/21/2018 10:26 AM, Guenter Roeck wrote:
> >On Wed, Feb 21, 2018 at 08:16:05AM -0800, Jae Hyun Yoo wrote:
> >>This commit adds a generic PECI hwmon client driver implementation.
> >>
> >>Signed-off-by: Jae Hyun Yoo 
> >>---
> >>  drivers/hwmon/Kconfig  |  10 +
> >>  drivers/hwmon/Makefile |   1 +
> >>  drivers/hwmon/peci-hwmon.c | 928 
> >> +
> >>  3 files changed, 939 insertions(+)
> >>  create mode 100644 drivers/hwmon/peci-hwmon.c
> >>
> >>diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >>index ef23553ff5cb..f22e0c31f597 100644
> >>--- a/drivers/hwmon/Kconfig
> >>+++ b/drivers/hwmon/Kconfig
> >>@@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
> >>  This driver can also be built as a module.  If so, the module
> >>  will be called nct7904.
> >>+config SENSORS_PECI_HWMON
> >>+   tristate "PECI hwmon support"
> >>+   depends on PECI
> >>+   help
> >>+ If you say yes here you get support for the generic PECI hwmon
> >>+ driver.
> >>+
> >>+ This driver can also be built as a module.  If so, the module
> >>+ will be called peci-hwmon.
> >>+
> >>  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 f814b4ace138..946f54b168e5 100644
> >>--- a/drivers/hwmon/Makefile
> >>+++ b/drivers/hwmon/Makefile
> >>@@ -135,6 +135,7 @@ 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_HWMON)   += peci-hwmon.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-hwmon.c b/drivers/hwmon/peci-hwmon.c
> >>new file mode 100644
> >>index ..edd27744adcb
> >>--- /dev/null
> >>+++ b/drivers/hwmon/peci-hwmon.c
> >>@@ -0,0 +1,928 @@
> >>+// SPDX-License-Identifier: GPL-2.0
> >>+// Copyright (c) 2018 Intel Corporation
> >>+
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+
> >>+#define DIMM_SLOT_NUMS_MAX12  /* Max DIMM numbers (channel ranks x 2) 
> >>*/
> >>+#define CORE_NUMS_MAX 28  /* Max core numbers (max on SKX 
> >>Platinum) */
> >>+#define TEMP_TYPE_PECI6   /* Sensor type 6: Intel PECI */
> >>+
> >>+#define CORE_TEMP_ATTRS   5
> >>+#define DIMM_TEMP_ATTRS   2
> >>+#define ATTR_NAME_LEN 24
> >>+
> >>+#define DEFAULT_ATTR_GRP_NUMS 5
> >>+
> >>+#define UPDATE_INTERVAL_MIN   HZ
> >>+#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
> >>+
> >>+enum sign {
> >>+   POS,
> >>+   NEG
> >>+};
> >>+
> >>+struct temp_data {
> >>+   bool valid;
> >>+   s32  value;
> >>+   unsigned long last_updated;
> >>+};
> >>+
> >>+struct temp_group {
> >>+   struct temp_data tjmax;
> >>+   struct temp_data tcontrol;
> >>+   struct temp_data tthrottle;
> >>+   struct temp_data dts_margin;
> >>+   struct temp_data die;
> >>+   struct temp_data core[CORE_NUMS_MAX];
> >>+   struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
> >>+};
> >>+
> >>+struct core_temp_group {
> >>+   struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
> >>+   char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
> >>+   struct attribute *attrs[CORE_TEMP_ATTRS + 1];
> >>+   struct attribute_group attr_group;
> >>+};
> >>+
> >>+struct dimm_temp_group {
> >>+   struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
> >>+   char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
> >>+   struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
> >>+   struct attribute_group attr_group;
> >>+};
> >>+
> >>+struct peci_hwmon {
> >>+   struct peci_client *client;
> >>+   struct device *dev;
> >>+   struct device *hwmon_dev;
> >>+   struct workqueue_struct *work_queue;
> >>+   struct delayed_work work_handler;
> >>+   char name[PECI_NAME_SIZE];
> >>+   struct temp_group temp;
> >>+   u8 addr;
> >>+   uint cpu_no;
> >>+   u32 core_mask;
> >>+   u32 dimm_mask;
> >>+   const struct attribute_group *core_attr_groups[CORE_NUMS_MAX + 1];
> >>+   const struct attribute_group *dimm_attr_groups[DIMM_SLOT_NUMS_MAX + 1];
> >>+   uint global_idx;
> >>+   uint core_idx;
> >>+   uint dimm_idx;
> >>+};
> >>+
> >>+enum label {
> >>+   L_DIE,
> >>+   L_DTS,
> >>+   L_TCONTROL,
> >>+   L_TTHROTTLE,
> >>+   L_TJMAX,
> >>+   L_MAX
> >>+};
> >>+
> >>+static const char *peci_label[L_MAX] = {
> >>+   "Die\n",
> >>+   "DTS margin to Tcontrol\n",
> >>+   "Tcontrol\n",
> >>+   "Tthrottle\n",
> >>+   "Tjmax\n",
> >>+};
> >>+
> >>+static int 

Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Guenter Roeck]

On Wed, Feb 21, 2018 at 08:16:05AM -0800, Jae Hyun Yoo wrote:
> This commit adds a generic PECI hwmon client driver implementation.
> 
> Signed-off-by: Jae Hyun Yoo 
> ---
>  drivers/hwmon/Kconfig  |  10 +
>  drivers/hwmon/Makefile |   1 +
>  drivers/hwmon/peci-hwmon.c | 928 
> +
>  3 files changed, 939 insertions(+)
>  create mode 100644 drivers/hwmon/peci-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index ef23553ff5cb..f22e0c31f597 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
> This driver can also be built as a module.  If so, the module
> will be called nct7904.
>  
> +config SENSORS_PECI_HWMON
> + tristate "PECI hwmon support"
> + depends on PECI
> + help
> +   If you say yes here you get support for the generic PECI hwmon
> +   driver.
> +
> +   This driver can also be built as a module.  If so, the module
> +   will be called peci-hwmon.
> +
>  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 f814b4ace138..946f54b168e5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -135,6 +135,7 @@ 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_HWMON) += peci-hwmon.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-hwmon.c b/drivers/hwmon/peci-hwmon.c
> new file mode 100644
> index ..edd27744adcb
> --- /dev/null
> +++ b/drivers/hwmon/peci-hwmon.c
> @@ -0,0 +1,928 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Intel Corporation
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DIMM_SLOT_NUMS_MAX12  /* Max DIMM numbers (channel ranks x 2) */
> +#define CORE_NUMS_MAX 28  /* Max core numbers (max on SKX Platinum) 
> */
> +#define TEMP_TYPE_PECI6   /* Sensor type 6: Intel PECI */
> +
> +#define CORE_TEMP_ATTRS   5
> +#define DIMM_TEMP_ATTRS   2
> +#define ATTR_NAME_LEN 24
> +
> +#define DEFAULT_ATTR_GRP_NUMS 5
> +
> +#define UPDATE_INTERVAL_MIN   HZ
> +#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
> +
> +enum sign {
> + POS,
> + NEG
> +};
> +
> +struct temp_data {
> + bool valid;
> + s32  value;
> + unsigned long last_updated;
> +};
> +
> +struct temp_group {
> + struct temp_data tjmax;
> + struct temp_data tcontrol;
> + struct temp_data tthrottle;
> + struct temp_data dts_margin;
> + struct temp_data die;
> + struct temp_data core[CORE_NUMS_MAX];
> + struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
> +};
> +
> +struct core_temp_group {
> + struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
> + char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
> + struct attribute *attrs[CORE_TEMP_ATTRS + 1];
> + struct attribute_group attr_group;
> +};
> +
> +struct dimm_temp_group {
> + struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
> + char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
> + struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
> + struct attribute_group attr_group;
> +};
> +
> +struct peci_hwmon {
> + struct peci_client *client;
> + struct device *dev;
> + struct device *hwmon_dev;
> + struct workqueue_struct *work_queue;
> + struct delayed_work work_handler;
> + char name[PECI_NAME_SIZE];
> + struct temp_group temp;
> + u8 addr;
> + uint cpu_no;
> + u32 core_mask;
> + u32 dimm_mask;
> + const struct attribute_group *core_attr_groups[CORE_NUMS_MAX + 1];
> + const struct attribute_group *dimm_attr_groups[DIMM_SLOT_NUMS_MAX + 1];
> + uint global_idx;
> + uint core_idx;
> + uint dimm_idx;
> +};
> +
> +enum label {
> + L_DIE,
> + L_DTS,
> + L_TCONTROL,
> + L_TTHROTTLE,
> + L_TJMAX,
> + L_MAX
> +};
> +
> +static const char *peci_label[L_MAX] = {
> + "Die\n",
> + "DTS margin to Tcontrol\n",
> + "Tcontrol\n",
> + "Tthrottle\n",
> + "Tjmax\n",
> +};
> +
> +static int send_peci_cmd(struct peci_hwmon *priv, enum peci_cmd cmd, void 
> *msg)
> +{
> + return peci_command(priv->client->adapter, cmd, msg);
> +}
> +
> +static int need_update(struct temp_data *temp)
> +{
> + if (temp->valid &&
> + time_before(jiffies, temp->last_updated + UPDATE_INTERVAL_MIN))
> + return 0;
> +
> + return 1;
> +}
> +
> +static s32 

[PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Jae Hyun Yoo]

This commit adds a generic PECI hwmon client driver implementation.

Signed-off-by: Jae Hyun Yoo 
---
 drivers/hwmon/Kconfig  |  10 +
 drivers/hwmon/Makefile |   1 +
 drivers/hwmon/peci-hwmon.c | 928 +
 3 files changed, 939 insertions(+)
 create mode 100644 drivers/hwmon/peci-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553ff5cb..f22e0c31f597 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
  This driver can also be built as a module.  If so, the module
  will be called nct7904.
 
+config SENSORS_PECI_HWMON
+   tristate "PECI hwmon support"
+   depends on PECI
+   help
+ If you say yes here you get support for the generic PECI hwmon
+ driver.
+
+ This driver can also be built as a module.  If so, the module
+ will be called peci-hwmon.
+
 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 f814b4ace138..946f54b168e5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -135,6 +135,7 @@ 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_HWMON)   += peci-hwmon.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-hwmon.c b/drivers/hwmon/peci-hwmon.c
new file mode 100644
index ..edd27744adcb
--- /dev/null
+++ b/drivers/hwmon/peci-hwmon.c
@@ -0,0 +1,928 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Intel Corporation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DIMM_SLOT_NUMS_MAX12  /* Max DIMM numbers (channel ranks x 2) */
+#define CORE_NUMS_MAX 28  /* Max core numbers (max on SKX Platinum) */
+#define TEMP_TYPE_PECI6   /* Sensor type 6: Intel PECI */
+
+#define CORE_TEMP_ATTRS   5
+#define DIMM_TEMP_ATTRS   2
+#define ATTR_NAME_LEN 24
+
+#define DEFAULT_ATTR_GRP_NUMS 5
+
+#define UPDATE_INTERVAL_MIN   HZ
+#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
+
+enum sign {
+   POS,
+   NEG
+};
+
+struct temp_data {
+   bool valid;
+   s32  value;
+   unsigned long last_updated;
+};
+
+struct temp_group {
+   struct temp_data tjmax;
+   struct temp_data tcontrol;
+   struct temp_data tthrottle;
+   struct temp_data dts_margin;
+   struct temp_data die;
+   struct temp_data core[CORE_NUMS_MAX];
+   struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
+};
+
+struct core_temp_group {
+   struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
+   char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
+   struct attribute *attrs[CORE_TEMP_ATTRS + 1];
+   struct attribute_group attr_group;
+};
+
+struct dimm_temp_group {
+   struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
+   char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
+   struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
+   struct attribute_group attr_group;
+};
+
+struct peci_hwmon {
+   struct peci_client *client;
+   struct device *dev;
+   struct device *hwmon_dev;
+   struct workqueue_struct *work_queue;
+   struct delayed_work work_handler;
+   char name[PECI_NAME_SIZE];
+   struct temp_group temp;
+   u8 addr;
+   uint cpu_no;
+   u32 core_mask;
+   u32 dimm_mask;
+   const struct attribute_group *core_attr_groups[CORE_NUMS_MAX + 1];
+   const struct attribute_group *dimm_attr_groups[DIMM_SLOT_NUMS_MAX + 1];
+   uint global_idx;
+   uint core_idx;
+   uint dimm_idx;
+};
+
+enum label {
+   L_DIE,
+   L_DTS,
+   L_TCONTROL,
+   L_TTHROTTLE,
+   L_TJMAX,
+   L_MAX
+};
+
+static const char *peci_label[L_MAX] = {
+   "Die\n",
+   "DTS margin to Tcontrol\n",
+   "Tcontrol\n",
+   "Tthrottle\n",
+   "Tjmax\n",
+};
+
+static int send_peci_cmd(struct peci_hwmon *priv, enum peci_cmd cmd, void *msg)
+{
+   return peci_command(priv->client->adapter, cmd, msg);
+}
+
+static int need_update(struct temp_data *temp)
+{
+   if (temp->valid &&
+   time_before(jiffies, temp->last_updated + UPDATE_INTERVAL_MIN))
+   return 0;
+
+   return 1;
+}
+
+static s32 ten_dot_six_to_millidegree(s32 x)
+{
+   return x) ^ 0x8000) - 0x8000) * 1000 / 64);
+}
+
+static int get_tjmax(struct peci_hwmon *priv)
+{
+   struct peci_rd_pkg_cfg_msg msg;
+   int rc;
+
+   if (!priv->temp.tjmax.valid) {
+   msg.addr = priv->addr;
+