Re: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On 1/11/2018 3:53 PM, Andrew Lunn wrote:
On Thu, Jan 11, 2018 at 03:14:37PM -0800, Jae Hyun Yoo wrote:
On 1/11/2018 2:18 PM, Andrew Lunn wrote:
+static const struct of_device_id peci_of_table[] = {
+ { .compatible = "peci-hwmon", },
This does not look like a reference to some piece of hardware.
This driver provides generic PECI hwmon function to which controller has
PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
specific hardware. Should I remove this or any suggestion?
PECI seems to be an Intel thing. So at least it should be
{ .compatible = "intel,peci-hwmon", }
assuming it is actually compatible with the Intel specification.
Andrew
Yes, PECI is an Intel thing but this driver is running on an ARM kernel on
Aspeed or Nuvoton chipsets for now. This driver will be monitoring a host
server's Intel CPU and DIMM which is running on a separated OS.
Hi Jae
You need to be careful with the name then. You should not claim the
name 'peci' in case somebody actually implements a PECI driver which
is compatible with Intel PECI.
However, looking at other comments, it seems like this part is going
away, if you turn your code into a bus driver.
Andrew
Hi Andrew,
I see. I'll keep that in mind and will keep finding if there is any
better way. Thanks for sharing your thought.
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On Thu, Jan 11, 2018 at 03:14:37PM -0800, Jae Hyun Yoo wrote:
> On 1/11/2018 2:18 PM, Andrew Lunn wrote:
> >+static const struct of_device_id peci_of_table[] = {
> >+{ .compatible = "peci-hwmon", },
>
> This does not look like a reference to some piece of hardware.
>
> >>>
> >>>This driver provides generic PECI hwmon function to which controller has
> >>>PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
> >>>specific hardware. Should I remove this or any suggestion?
> >
> >PECI seems to be an Intel thing. So at least it should be
> >
> > { .compatible = "intel,peci-hwmon", }
> >
> >assuming it is actually compatible with the Intel specification.
> >
> > Andrew
> >
>
> Yes, PECI is an Intel thing but this driver is running on an ARM kernel on
> Aspeed or Nuvoton chipsets for now. This driver will be monitoring a host
> server's Intel CPU and DIMM which is running on a separated OS.
Hi Jae
You need to be careful with the name then. You should not claim the
name 'peci' in case somebody actually implements a PECI driver which
is compatible with Intel PECI.
However, looking at other comments, it seems like this part is going
away, if you turn your code into a bus driver.
Andrew
--
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On 1/11/2018 2:18 PM, Andrew Lunn wrote:
+static const struct of_device_id peci_of_table[] = {
+ { .compatible = "peci-hwmon", },
This does not look like a reference to some piece of hardware.
This driver provides generic PECI hwmon function to which controller has
PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
specific hardware. Should I remove this or any suggestion?
PECI seems to be an Intel thing. So at least it should be
{ .compatible = "intel,peci-hwmon", }
assuming it is actually compatible with the Intel specification.
Andrew
Yes, PECI is an Intel thing but this driver is running on an ARM kernel
on Aspeed or Nuvoton chipsets for now. This driver will be monitoring a
host server's Intel CPU and DIMM which is running on a separated OS.
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On 1/11/2018 1:40 PM, Guenter Roeck wrote:
On Thu, Jan 11, 2018 at 11:47:01AM -0800, Jae Hyun Yoo wrote:
On 1/10/2018 1:47 PM, Guenter Roeck wrote:
On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
This commit adds driver implementation for a generic PECI hwmon.
Signed-off-by: Jae Hyun Yoo
[ ... ]
+
+ if (priv->temp.tcontrol.valid &&
+ time_before(jiffies, priv->temp.tcontrol.last_updated +
+UPDATE_INTERVAL_MIN))
+ return 0;
+
Is the delay necessary ? Otherwise I would suggest to drop it.
It adds a lot of complexity to the driver. Also, if the user polls
values more often, that is presumably on purpose.
I was intended to reduce traffic on PECI bus because it's low speed single
wired bus, and temperature values don't change frequently because the value
is sampled and averaged in CPU itself. I'll keep this.
Then please try to move the common code into a single function.
That makes sense. I'll move the common code into a single function.
[ ... ]
+
+ rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
What entity determines cpu-id ?
CPU ID numbering is determined by hardware SOCKET_ID strap pins. In this
driver implementation, cpu-id is being used as CPU client indexing.
Seems to me the necessary information to identify a given CPU should
be provided by the PECI core. Also, there are already "cpu" nodes
in devicetree which, if I recall correctly, may include information
such as CPU Ids.
This driver is implemented to support only BMC (Baseboard Management
Controllers) chipset which is running on a separated linux kernel from a
host server system. Through a PECI connection between them, this driver
collects the host server system's CPU and DIMM temperature information
which is running on a separated OS. That could be a linux, a Windows OS
or any other OSes. I mean, there is no shared devicetree data between
them so it is why the 'cpu_id' was added into dt configuration of this
driver.
Using quite limited hardware connections such as PECI, KSC, eSPI and
SMBus, the BMC manages the host server and this hwmon is one of features
of BMC.
+ if (rc || priv->cpu_id >= CPU_ID_MAX) {
+ dev_err(dev, "Invalid cpu-id configuration\n");
+ return rc;
+ }
+
+ rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
This is an odd devicetree attribute. Normally the number of DIMMs
is dynamic. Isn't there a means to get all that information dynamically
instead of having to set it through devicetree ? What if someone adds
or removes a DIMM ? Who updates the devicetree ?
It means the number of DIMM slots each CPU has, doesn't mean the number of
currently installed DIMM components. If a DIMM is inserted a slot, CPU
reports its actual temperature but on empty slot, CPU reports 0 instead of
reporting an error so it is the reason why this driver enumerates all DIMM
slots' attribute.
And there is no other means to get the number of DIMM slots per CPU ?
It just seems to be that this is the wrong location to provide such
information.
This devicetree attribute will be configured at runtime using dt overlay
based on the host server's hardware configuration which will be parsed
and managed by a userspace BMC service.
[ ... ]
+
+static const struct of_device_id peci_of_table[] = {
+ { .compatible = "peci-hwmon", },
This does not look like a reference to some piece of hardware.
This driver provides generic PECI hwmon function to which controller has
PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
specific hardware. Should I remove this or any suggestion?
I don't really know enough about the system to make a recommendation.
It seems to me that the PECI core should identify which functionality
it supports and instantiate the necessary driver(s). Maybe there should
be sub-nodes to the peci node with relevant information. Those sub-nodes
should specify the supported functionality in more detail, though -
such as indicating the supported CPU and/or DIMM sensors.
Guenter
As I explained above, BMC and host server are running on separated OSes
so this driver cannot be (actually, doesn't need to be) directly
associated with other kernel modules in the BMC side OS for identifying
the host server system's functionality. My thought is, this driver will
use PECI only for identifying the host server's CPU and DIMM information
and the userspace BMC service could deliver any additional host server
information thru dt overlay if needed before the BMC service initiates
this driver at runtime.
Thanks a lot,
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
> > >>+static const struct of_device_id peci_of_table[] = {
> > >>+ { .compatible = "peci-hwmon", },
> > >
> > >This does not look like a reference to some piece of hardware.
> > >
> >
> > This driver provides generic PECI hwmon function to which controller has
> > PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
> > specific hardware. Should I remove this or any suggestion?
PECI seems to be an Intel thing. So at least it should be
{ .compatible = "intel,peci-hwmon", }
assuming it is actually compatible with the Intel specification.
Andrew
--
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On Thu, Jan 11, 2018 at 11:47:01AM -0800, Jae Hyun Yoo wrote:
> On 1/10/2018 1:47 PM, Guenter Roeck wrote:
> >On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
> >>This commit adds driver implementation for a generic PECI hwmon.
> >>
> >>Signed-off-by: Jae Hyun Yoo
[ ... ]
> >>+
> >>+ if (priv->temp.tcontrol.valid &&
> >>+ time_before(jiffies, priv->temp.tcontrol.last_updated +
> >>+UPDATE_INTERVAL_MIN))
> >>+ return 0;
> >>+
> >
> >Is the delay necessary ? Otherwise I would suggest to drop it.
> >It adds a lot of complexity to the driver. Also, if the user polls
> >values more often, that is presumably on purpose.
> >
>
> I was intended to reduce traffic on PECI bus because it's low speed single
> wired bus, and temperature values don't change frequently because the value
> is sampled and averaged in CPU itself. I'll keep this.
>
Then please try to move the common code into a single function.
[ ... ]
> >>+
> >>+ rc = of_property_read_u32(np, "cpu-id", &priv->cpu_id);
> >
> >What entity determines cpu-id ?
> >
>
> CPU ID numbering is determined by hardware SOCKET_ID strap pins. In this
> driver implementation, cpu-id is being used as CPU client indexing.
>
Seems to me the necessary information to identify a given CPU should
be provided by the PECI core. Also, there are already "cpu" nodes
in devicetree which, if I recall correctly, may include information
such as CPU Ids.
> >>+ if (rc || priv->cpu_id >= CPU_ID_MAX) {
> >>+ dev_err(dev, "Invalid cpu-id configuration\n");
> >>+ return rc;
> >>+ }
> >>+
> >>+ rc = of_property_read_u32(np, "dimm-nums", &priv->dimm_nums);
> >
> >This is an odd devicetree attribute. Normally the number of DIMMs
> >is dynamic. Isn't there a means to get all that information dynamically
> >instead of having to set it through devicetree ? What if someone adds
> >or removes a DIMM ? Who updates the devicetree ?
> >
>
> It means the number of DIMM slots each CPU has, doesn't mean the number of
> currently installed DIMM components. If a DIMM is inserted a slot, CPU
> reports its actual temperature but on empty slot, CPU reports 0 instead of
> reporting an error so it is the reason why this driver enumerates all DIMM
> slots' attribute.
>
And there is no other means to get the number of DIMM slots per CPU ?
It just seems to be that this is the wrong location to provide such
information.
[ ... ]
> >>+
> >>+static const struct of_device_id peci_of_table[] = {
> >>+ { .compatible = "peci-hwmon", },
> >
> >This does not look like a reference to some piece of hardware.
> >
>
> This driver provides generic PECI hwmon function to which controller has
> PECI HW such as Aspeed or Nuvoton BMC chip so it's not dependant on a
> specific hardware. Should I remove this or any suggestion?
>
I don't really know enough about the system to make a recommendation.
It seems to me that the PECI core should identify which functionality
it supports and instantiate the necessary driver(s). Maybe there should
be sub-nodes to the peci node with relevant information. Those sub-nodes
should specify the supported functionality in more detail, though -
such as indicating the supported CPU and/or DIMM sensors.
Guenter
--
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 linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On 1/11/2018 5:22 AM, Arnd Bergmann wrote:
On Thu, Jan 11, 2018 at 12:45 AM, Jae Hyun Yoo
wrote:
On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
wrote:
This commit adds driver implementation for a generic PECI hwmon.
Signed-off-by: Jae Hyun Yoo
+static int xfer_peci_msg(int cmd, void *pmsg)
+{
+ int rc;
+
+ mutex_lock(&peci_hwmon_lock);
+ rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
+ mutex_unlock(&peci_hwmon_lock);
+
+ return rc;
+}
I said earlier that peci_ioctl() looked unused, that was obviously
wrong, but what you have here
is not a proper way to abstract a bus.
Maybe this can be done more like an i2c bus: make the peci controller
a bus device
and register all known target/index pairs as devices with the peci bus
type, and have
them probed from DT. The driver can then bind to each of those
individually.
Not sure if that is getting to granular at that point, I'd have to
understand better
how it is expected to get used, and what the variances are between
implementations.
Thanks for sharing your opinion. In fact, this was also suggested by openbmc
community so I should consider of redesigning it. I'm currently thinking
about adding a new PECI device class as an abstract layer and any BMC
chipset specific driver could be attached to the PECI class driver. Then,
each CPU client could be registered as an individual device as you
suggested. Will consider your suggestion.
Another idea might be to pretend that PECI was I2C. We already have a few
drivers for hardware that is not I2C but whose software interface looks
similar enough that it just works. No idea if that is the case for PECI, but
xfer_peci_msg might be close enough to i2c_xfer to make it work. If you
are able to do that, then the PECI controller would just register itself
as an i2c controller and it can be accessed using /dev/i2c from user space
or a high-level i2c_driver.
Arnd
Thanks for the good idea. It looks like one of possible options. I'll
check this idea as well. :)
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On 1/10/2018 1:47 PM, Guenter Roeck wrote:
On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
This commit adds driver implementation for a generic PECI hwmon.
Signed-off-by: Jae Hyun Yoo
---
drivers/hwmon/Kconfig | 6 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/peci-hwmon.c | 953 +
3 files changed, 960 insertions(+)
create mode 100644 drivers/hwmon/peci-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9256dd0..3a62c60 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1234,6 +1234,12 @@ 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 ASPEED_PECI
+ help
+ If you say yes here you get support for the generic PECI hwmon driver.
+
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 98000fc..41d43a5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -131,6 +131,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 000..2d2a288
--- /dev/null
+++ b/drivers/hwmon/peci-hwmon.c
@@ -0,0 +1,953 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Intel Corporation
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
misc, not linux ? That seems wrong.
You are right. I'll fix it.
+
+#define DEVICE_NAME "peci-hwmon"
+#define HWMON_NAME "peci_hwmon"
+
+#define CPU_ID_MAX 8 /* Max CPU number configured by socket ID */
+#define DIMM_NUMS_MAX16 /* Max DIMM numbers (channel ranks x 2) */
+#define CORE_NUMS_MAX28 /* Max core numbers (max on SKX Platinum) */
I won't insist, but it would be better if this were dynamic,
otherwise we'll end up having to increase the defines in the future.
Right. As you said, these values should be manually adjusted in the
future if CPU architecture has been changed so better implement it as
dynamic. I will check again a way of getting these values from client
CPU thru PECI connection.
+#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */
+#define CORE_INDEX_OFFSET100 /* sysfs filename start offset for core temp
*/
+#define DIMM_INDEX_OFFSET200 /* sysfs filename start offset for DIMM temp
*/
Did you test with the "sensors" command to ensure that this works,
with the large gaps in index values ?
Overall, I am not very happy with the indexing. Since each sensor as
a label, it might be better to just make it dynamic.
Okay, that makes sense. Since all attributes has its own label, indexing
gap wouldn't be needed even in case of CPU architecture change happens.
I'll remove the indexing gap.
+#define TEMP_NAME_HEADER_LEN 4 /* sysfs temp type header length */
+#define OF_DIMM_NUMS_DEFAULT 16 /* default dimm-nums setting */
+
+#define CORE_TEMP_ATTRS 5
+#define DIMM_TEMP_ATTRS 2
+#define ATTR_NAME_LEN24
+
+#define UPDATE_INTERVAL_MIN HZ
+
+enum sign_t {
+ POS,
+ NEG
+};
+
+struct cpuinfo_t {
+ bool valid;
+ u32 dib;
+ u8 cpuid;
+ u8 platform_id;
+ u32 microcode;
+ u8 logical_thread_nums;
+};
+
+struct temp_data_t {
+ bool valid;
+ s32 value;
+ unsigned long last_updated;
+};
+
+struct temp_group_t {
+ struct temp_data_t tjmax;
+ struct temp_data_t tcontrol;
+ struct temp_data_t tthrottle;
+ struct temp_data_t dts_margin;
+ struct temp_data_t die;
+ struct temp_data_t core[CORE_NUMS_MAX];
+ struct temp_data_t dimm[DIMM_NUMS_MAX];
+};
+
+struct core_temp_attr_group_t {
+ struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
+ char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
+ struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
+ struct attribute_group attr_group[CORE_NUMS_MAX];
+};
+
+struct dimm_temp_attr_group_t {
+ struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
+ char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
+ struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
+ struct attribute_group attr_group[DIMM_NUMS_MAX];
+};
+
+struct peci_hwmon {
+ struct device *dev;
Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On Thu, Jan 11, 2018 at 12:45 AM, Jae Hyun Yoo
wrote:
> On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
>>
>> On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
>> wrote:
>>>
>>> This commit adds driver implementation for a generic PECI hwmon.
>>>
>>> Signed-off-by: Jae Hyun Yoo
>>
>>
>>> +static int xfer_peci_msg(int cmd, void *pmsg)
>>> +{
>>> + int rc;
>>> +
>>> + mutex_lock(&peci_hwmon_lock);
>>> + rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
>>> + mutex_unlock(&peci_hwmon_lock);
>>> +
>>> + return rc;
>>> +}
>>
>>
>> I said earlier that peci_ioctl() looked unused, that was obviously
>> wrong, but what you have here
>> is not a proper way to abstract a bus.
>>
>> Maybe this can be done more like an i2c bus: make the peci controller
>> a bus device
>> and register all known target/index pairs as devices with the peci bus
>> type, and have
>> them probed from DT. The driver can then bind to each of those
>> individually.
>> Not sure if that is getting to granular at that point, I'd have to
>> understand better
>> how it is expected to get used, and what the variances are between
>> implementations.
>>
>
> Thanks for sharing your opinion. In fact, this was also suggested by openbmc
> community so I should consider of redesigning it. I'm currently thinking
> about adding a new PECI device class as an abstract layer and any BMC
> chipset specific driver could be attached to the PECI class driver. Then,
> each CPU client could be registered as an individual device as you
> suggested. Will consider your suggestion.
Another idea might be to pretend that PECI was I2C. We already have a few
drivers for hardware that is not I2C but whose software interface looks
similar enough that it just works. No idea if that is the case for PECI, but
xfer_peci_msg might be close enough to i2c_xfer to make it work. If you
are able to do that, then the PECI controller would just register itself
as an i2c controller and it can be accessed using /dev/i2c from user space
or a high-level i2c_driver.
Arnd
--
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 linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On 1/10/2018 4:29 AM, Arnd Bergmann wrote:
On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
wrote:
This commit adds driver implementation for a generic PECI hwmon.
Signed-off-by: Jae Hyun Yoo
+static int xfer_peci_msg(int cmd, void *pmsg)
+{
+ int rc;
+
+ mutex_lock(&peci_hwmon_lock);
+ rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
+ mutex_unlock(&peci_hwmon_lock);
+
+ return rc;
+}
I said earlier that peci_ioctl() looked unused, that was obviously
wrong, but what you have here
is not a proper way to abstract a bus.
Maybe this can be done more like an i2c bus: make the peci controller
a bus device
and register all known target/index pairs as devices with the peci bus
type, and have
them probed from DT. The driver can then bind to each of those individually.
Not sure if that is getting to granular at that point, I'd have to
understand better
how it is expected to get used, and what the variances are between
implementations.
Arnd
Thanks for sharing your opinion. In fact, this was also suggested by
openbmc community so I should consider of redesigning it. I'm currently
thinking about adding a new PECI device class as an abstract layer and
any BMC chipset specific driver could be attached to the PECI class
driver. Then, each CPU client could be registered as an individual
device as you suggested. Will consider your suggestion.
Thanks a lot!
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: [linux, dev-4.10, 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On Tue, Jan 09, 2018 at 02:31:26PM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for a generic PECI hwmon.
>
> Signed-off-by: Jae Hyun Yoo
> ---
> drivers/hwmon/Kconfig | 6 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/peci-hwmon.c | 953
> +
> 3 files changed, 960 insertions(+)
> create mode 100644 drivers/hwmon/peci-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 9256dd0..3a62c60 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1234,6 +1234,12 @@ 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 ASPEED_PECI
> + help
> + If you say yes here you get support for the generic PECI hwmon driver.
> +
> 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 98000fc..41d43a5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -131,6 +131,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 000..2d2a288
> --- /dev/null
> +++ b/drivers/hwmon/peci-hwmon.c
> @@ -0,0 +1,953 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017 Intel Corporation
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
misc, not linux ? That seems wrong.
> +
> +#define DEVICE_NAME "peci-hwmon"
> +#define HWMON_NAME "peci_hwmon"
> +
> +#define CPU_ID_MAX 8 /* Max CPU number configured by socket ID */
> +#define DIMM_NUMS_MAX16 /* Max DIMM numbers (channel ranks x 2) */
> +#define CORE_NUMS_MAX28 /* Max core numbers (max on SKX Platinum) */
I won't insist, but it would be better if this were dynamic,
otherwise we'll end up having to increase the defines in the future.
> +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */
> +#define CORE_INDEX_OFFSET100 /* sysfs filename start offset for core
> temp */
> +#define DIMM_INDEX_OFFSET200 /* sysfs filename start offset for DIMM
> temp */
Did you test with the "sensors" command to ensure that this works,
with the large gaps in index values ?
Overall, I am not very happy with the indexing. Since each sensor as
a label, it might be better to just make it dynamic.
> +#define TEMP_NAME_HEADER_LEN 4 /* sysfs temp type header length */
> +#define OF_DIMM_NUMS_DEFAULT 16 /* default dimm-nums setting */
> +
> +#define CORE_TEMP_ATTRS 5
> +#define DIMM_TEMP_ATTRS 2
> +#define ATTR_NAME_LEN24
> +
> +#define UPDATE_INTERVAL_MIN HZ
> +
> +enum sign_t {
> + POS,
> + NEG
> +};
> +
> +struct cpuinfo_t {
> + bool valid;
> + u32 dib;
> + u8 cpuid;
> + u8 platform_id;
> + u32 microcode;
> + u8 logical_thread_nums;
> +};
> +
> +struct temp_data_t {
> + bool valid;
> + s32 value;
> + unsigned long last_updated;
> +};
> +
> +struct temp_group_t {
> + struct temp_data_t tjmax;
> + struct temp_data_t tcontrol;
> + struct temp_data_t tthrottle;
> + struct temp_data_t dts_margin;
> + struct temp_data_t die;
> + struct temp_data_t core[CORE_NUMS_MAX];
> + struct temp_data_t dimm[DIMM_NUMS_MAX];
> +};
> +
> +struct core_temp_attr_group_t {
> + struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
> + char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
> + struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
> + struct attribute_group attr_group[CORE_NUMS_MAX];
> +};
> +
> +struct dimm_temp_attr_group_t {
> + struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
> + char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
> + struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
> + struct attribute_group attr_group[DIMM_NUMS_MAX];
> +};
> +
> +struct peci_hwmon {
> + struct device *dev;
> + struct device *hwmon_dev;
> + char name[NAME_MAX];
> + const struct attribute_group **groups;
> + struct cpuinfo_t cpuinfo;
> + struct temp_group_t temp;
> + u32 cpu_id;
> + bool show_core;
> + u32 core_nums;
> + u32 dimm_nums;
> + atomic_t core_group_created;
> +
Re: [PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
On Tue, Jan 9, 2018 at 11:31 PM, Jae Hyun Yoo
wrote:
> This commit adds driver implementation for a generic PECI hwmon.
>
> Signed-off-by: Jae Hyun Yoo
> +static int xfer_peci_msg(int cmd, void *pmsg)
> +{
> + int rc;
> +
> + mutex_lock(&peci_hwmon_lock);
> + rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg);
> + mutex_unlock(&peci_hwmon_lock);
> +
> + return rc;
> +}
I said earlier that peci_ioctl() looked unused, that was obviously
wrong, but what you have here
is not a proper way to abstract a bus.
Maybe this can be done more like an i2c bus: make the peci controller
a bus device
and register all known target/index pairs as devices with the peci bus
type, and have
them probed from DT. The driver can then bind to each of those individually.
Not sure if that is getting to granular at that point, I'd have to
understand better
how it is expected to get used, and what the variances are between
implementations.
Arnd
--
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
[PATCH linux dev-4.10 6/6] drivers/hwmon: Add a driver for a generic PECI hwmon
This commit adds driver implementation for a generic PECI hwmon.
Signed-off-by: Jae Hyun Yoo
---
drivers/hwmon/Kconfig | 6 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/peci-hwmon.c | 953 +
3 files changed, 960 insertions(+)
create mode 100644 drivers/hwmon/peci-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9256dd0..3a62c60 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1234,6 +1234,12 @@ 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 ASPEED_PECI
+ help
+ If you say yes here you get support for the generic PECI hwmon driver.
+
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 98000fc..41d43a5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -131,6 +131,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 000..2d2a288
--- /dev/null
+++ b/drivers/hwmon/peci-hwmon.c
@@ -0,0 +1,953 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Intel Corporation
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEVICE_NAME "peci-hwmon"
+#define HWMON_NAME "peci_hwmon"
+
+#define CPU_ID_MAX 8 /* Max CPU number configured by socket ID */
+#define DIMM_NUMS_MAX16 /* Max DIMM numbers (channel ranks x 2) */
+#define CORE_NUMS_MAX28 /* Max core numbers (max on SKX Platinum) */
+#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */
+#define CORE_INDEX_OFFSET100 /* sysfs filename start offset for core temp
*/
+#define DIMM_INDEX_OFFSET200 /* sysfs filename start offset for DIMM temp
*/
+#define TEMP_NAME_HEADER_LEN 4 /* sysfs temp type header length */
+#define OF_DIMM_NUMS_DEFAULT 16 /* default dimm-nums setting */
+
+#define CORE_TEMP_ATTRS 5
+#define DIMM_TEMP_ATTRS 2
+#define ATTR_NAME_LEN24
+
+#define UPDATE_INTERVAL_MIN HZ
+
+enum sign_t {
+ POS,
+ NEG
+};
+
+struct cpuinfo_t {
+ bool valid;
+ u32 dib;
+ u8 cpuid;
+ u8 platform_id;
+ u32 microcode;
+ u8 logical_thread_nums;
+};
+
+struct temp_data_t {
+ bool valid;
+ s32 value;
+ unsigned long last_updated;
+};
+
+struct temp_group_t {
+ struct temp_data_t tjmax;
+ struct temp_data_t tcontrol;
+ struct temp_data_t tthrottle;
+ struct temp_data_t dts_margin;
+ struct temp_data_t die;
+ struct temp_data_t core[CORE_NUMS_MAX];
+ struct temp_data_t dimm[DIMM_NUMS_MAX];
+};
+
+struct core_temp_attr_group_t {
+ struct sensor_device_attribute sd_attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS];
+ char attr_name[CORE_NUMS_MAX][CORE_TEMP_ATTRS][ATTR_NAME_LEN];
+ struct attribute *attrs[CORE_NUMS_MAX][CORE_TEMP_ATTRS + 1];
+ struct attribute_group attr_group[CORE_NUMS_MAX];
+};
+
+struct dimm_temp_attr_group_t {
+ struct sensor_device_attribute sd_attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS];
+ char attr_name[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
+ struct attribute *attrs[DIMM_NUMS_MAX][DIMM_TEMP_ATTRS + 1];
+ struct attribute_group attr_group[DIMM_NUMS_MAX];
+};
+
+struct peci_hwmon {
+ struct device *dev;
+ struct device *hwmon_dev;
+ char name[NAME_MAX];
+ const struct attribute_group **groups;
+ struct cpuinfo_t cpuinfo;
+ struct temp_group_t temp;
+ u32 cpu_id;
+ bool show_core;
+ u32 core_nums;
+ u32 dimm_nums;
+ atomic_t core_group_created;
+ struct core_temp_attr_group_t core;
+ struct dimm_temp_attr_group_t dimm;
+};
+
+enum label_t {
+ L_DIE,
+ L_DTS,
+ L_TCONTROL,
+ L_TTHROTTLE,
+ L_MAX
+};
+
+static const char *peci_label[L_MAX] = {
+ "Die temperature\n",
+ "DTS thermal margin to Tcontrol\n",
+ "Tcontrol temperature\n",
+ "Tthrottle temperature\n",
+};
+
+static DEFINE_MUTEX(peci_hwmon_lock);
+
+static int create_core_temp_group(struct peci_hwmon *priv, int core_no);
+
+
+static int xfer_peci_msg(int cmd, void *pmsg)
+{
+ int rc;
+
+ mutex_lock(&peci_hwmon_lock);
+ rc = peci_ioctl(NULL, cmd, (unsigned long)pmsg
