Hi Greg,
This is a second round of patches introducing PECI subsystem.
I don't think it is ready to be applied right away (we're still
missing r-b's), but I hope we have chance to complete discussion in
the 5.15 development cycle. I would appreciate if you could take
a look.
Note: All changes to arch/x86 are contained within patches 01-02, plus
small Kconfig change adding "depends on PECI" to GENERIC_LIB_X86
Kconfig in patch 10.
The Platform Environment Control Interface (PECI) is a communication
interface between Intel processors and management controllers (e.g.
Baseboard Management Controller, BMC).
This series adds a PECI subsystem and introduces drivers which run in
the Linux instance on the management controller (not the main Intel
processor) and is intended to be used by the OpenBMC [1], a Linux
distribution for BMC devices.
The information exposed over PECI (like processor and DIMM
temperature) refers to the Intel processor and can be consumed by
daemons running on the BMC to, for example, display the processor
temperature in its web interface.
The PECI bus is collection of code that provides interface support
between PECI devices (that actually represent processors) and PECI
controllers (such as the "peci-aspeed" controller) that allow to
access physical PECI interface. PECI devices are bound to PECI
drivers that provides access to PECI services. This series introduces
a generic "peci-cpu" driver that exposes hardware monitoring "cputemp"
and "dimmtemp" using the auxiliary bus.
Exposing "raw" PECI to userspace, either to write userspace drivers or
for debug/testing purpose was left out of this series to encourage
writing kernel drivers instead, but may be pursued in the future.
Introducing PECI to upstream Linux was already attempted before [2].
Since it's been over a year since last revision, and the series
changed quite a bit in the meantime, I've decided to start from v1.
I would also like to give credit to everyone who helped me with
different aspects of preliminary review:
- Pierre-Louis Bossart,
- Tony Luck,
- Andy Shevchenko,
- Dave Hansen.
[1] https://github.com/openbmc/openbmc
[2] https://lore.kernel.org/openbmc/20191211194624.2872-1-jae.hyun.yoo@linux.intel.com/
Changes v1 -> v2:
Biggest changes when it comes to diffstat are locking in HWMON
(I decided to clean things up a bit while adding it), switching to
devres usage in more places and exposing sysfs interface in separate patch.
* Moved extending X86 ARCHITECTURE MAINTAINERS earlier in series (Dan)
* Removed "default n" for GENERIC_LIB_X86 (Dan)
* Added vendor prefix for peci-aspeed specific properties (Rob)
* Refactored PECI to use devres consistently (Dan)
* Added missing sysfs documentation and excluded adding peci-sysfs to
separate patch (Dan)
* Used module_init() instead of subsys_init() for peci module initialization (Dan)
* Removed redundant struct peci_device member (Dan)
* Improved PECI Kconfig help (Randy/Dan)
* Fixed/removed log messages (Dan, Guenter)
* Refactored peci-cputemp and peci-dimmtemp and added missing locks (Guenter)
* Removed unused dev_set_drvdata() in peci-cputemp and peci-dimmtemp (Guenter)
* Fixed used types, names, fixed broken and added additional comments
to peci-hwmon (Guenter, Zev)
* Refactored peci-dimmtemp to not return -ETIMEDOUT (Guenter)
* Added sanity check for min_peci_revision in peci-hwmon drivers (Zev)
* Added assert for DIMM_NUMS_MAX and additional warning in peci-dimmtemp (Zev)
* Fixed macro names in peci-aspeed (Zev)
* Refactored peci-aspeed sanitizing properties to a single helper function (Zev)
* Fixed peci_cpu_device_ids definition for Broadwell Xeon D (David)
* Refactor peci_request to use a single allocation (Zev)
* Used min_t() to improve code readability (Zev)
* Added macro for PECI_RDENDPTCFG_MMIO_WR_LEN_BASE and fixed adev type
array name to more descriptive (Zev)
* Fixed peci-hwmon commit-msg and documentation (Zev)
Thanks
-Iwona
Iwona Winiarska (13):
x86/cpu: Move intel-family to arch-independent headers
x86/cpu: Extract cpuid helpers to arch-independent
dt-bindings: Add generic bindings for PECI
dt-bindings: Add bindings for peci-aspeed
ARM: dts: aspeed: Add PECI controller nodes
peci: Add core infrastructure
peci: Add device detection
peci: Add sysfs interface for PECI bus
peci: Add support for PECI device drivers
peci: Add peci-cpu driver
hwmon: peci: Add cputemp driver
hwmon: peci: Add dimmtemp driver
docs: Add PECI documentation
Jae Hyun Yoo (2):
peci: Add peci-aspeed controller driver
docs: hwmon: Document PECI drivers
Documentation/ABI/testing/sysfs-bus-peci | 16 +
.../devicetree/bindings/peci/peci-aspeed.yaml | 109 ++++
.../bindings/peci/peci-controller.yaml | 33 +
Documentation/hwmon/index.rst | 2 +
Documentation/hwmon/peci-cputemp.rst | 90 +++
Documentation/hwmon/peci-dimmtemp.rst | 57 ++
Documentation/index.rst | 1 +
Documentation/peci/index.rst | 16 +
Documentation/peci/peci.rst | 48 ++
MAINTAINERS | 32 +
arch/arm/boot/dts/aspeed-g4.dtsi | 14 +
arch/arm/boot/dts/aspeed-g5.dtsi | 14 +
arch/arm/boot/dts/aspeed-g6.dtsi | 14 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/cpu.h | 3 -
arch/x86/include/asm/intel-family.h | 141 +---
arch/x86/include/asm/microcode.h | 2 +-
arch/x86/kvm/cpuid.h | 3 +-
arch/x86/lib/Makefile | 2 +-
drivers/Kconfig | 3 +
drivers/Makefile | 1 +
drivers/edac/mce_amd.c | 3 +-
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/peci/Kconfig | 31 +
drivers/hwmon/peci/Makefile | 7 +
drivers/hwmon/peci/common.h | 58 ++
drivers/hwmon/peci/cputemp.c | 591 +++++++++++++++++
drivers/hwmon/peci/dimmtemp.c | 614 ++++++++++++++++++
drivers/peci/Kconfig | 37 ++
drivers/peci/Makefile | 10 +
drivers/peci/controller/Kconfig | 16 +
drivers/peci/controller/Makefile | 3 +
drivers/peci/controller/peci-aspeed.c | 445 +++++++++++++
drivers/peci/core.c | 238 +++++++
drivers/peci/cpu.c | 344 ++++++++++
drivers/peci/device.c | 221 +++++++
drivers/peci/internal.h | 137 ++++
drivers/peci/request.c | 477 ++++++++++++++
drivers/peci/sysfs.c | 82 +++
include/linux/peci-cpu.h | 38 ++
include/linux/peci.h | 110 ++++
include/linux/x86/cpu.h | 9 +
include/linux/x86/intel-family.h | 146 +++++
lib/Kconfig | 4 +
lib/Makefile | 2 +
lib/x86/Makefile | 3 +
{arch/x86/lib => lib/x86}/cpu.c | 2 +-
48 files changed, 4084 insertions(+), 149 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-peci
create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.yaml
create mode 100644 Documentation/devicetree/bindings/peci/peci-controller.yaml
create mode 100644 Documentation/hwmon/peci-cputemp.rst
create mode 100644 Documentation/hwmon/peci-dimmtemp.rst
create mode 100644 Documentation/peci/index.rst
create mode 100644 Documentation/peci/peci.rst
create mode 100644 drivers/hwmon/peci/Kconfig
create mode 100644 drivers/hwmon/peci/Makefile
create mode 100644 drivers/hwmon/peci/common.h
create mode 100644 drivers/hwmon/peci/cputemp.c
create mode 100644 drivers/hwmon/peci/dimmtemp.c
create mode 100644 drivers/peci/Kconfig
create mode 100644 drivers/peci/Makefile
create mode 100644 drivers/peci/controller/Kconfig
create mode 100644 drivers/peci/controller/Makefile
create mode 100644 drivers/peci/controller/peci-aspeed.c
create mode 100644 drivers/peci/core.c
create mode 100644 drivers/peci/cpu.c
create mode 100644 drivers/peci/device.c
create mode 100644 drivers/peci/internal.h
create mode 100644 drivers/peci/request.c
create mode 100644 drivers/peci/sysfs.c
create mode 100644 include/linux/peci-cpu.h
create mode 100644 include/linux/peci.h
create mode 100644 include/linux/x86/cpu.h
create mode 100644 include/linux/x86/intel-family.h
create mode 100644 lib/x86/Makefile
rename {arch/x86/lib => lib/x86}/cpu.c (95%)
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
Move the data from arch/x86/include/asm/intel-family.h into a new file
include/linux/x86/intel-family.h so that it can be used by other
architectures.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
To limit tree-wide changes and help people that were expecting
intel-family defines in arch/x86 to find it more easily without going
through git history, we're not removing the original header
completely, we're keeping it as a "stub" that includes the new one.
If there is a consensus that the tree-wide option is better,
we can choose this approach.
MAINTAINERS | 2 +
arch/x86/include/asm/intel-family.h | 141 +--------------------------
include/linux/x86/intel-family.h | 146 ++++++++++++++++++++++++++++
3 files changed, 149 insertions(+), 140 deletions(-)
create mode 100644 include/linux/x86/intel-family.h
On Tue, Aug 03, 2021 at 01:31:20PM +0200, Iwona Winiarska wrote:
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
Move the data from arch/x86/include/asm/intel-family.h into a new file
include/linux/x86/intel-family.h so that it can be used by other
architectures.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
To limit tree-wide changes and help people that were expecting
intel-family defines in arch/x86 to find it more easily without going
through git history, we're not removing the original header
completely, we're keeping it as a "stub" that includes the new one.
If there is a consensus that the tree-wide option is better,
we can choose this approach.
Why can't the linux/ namespace header include the x86 one so that
nothing changes for arch/x86/?
And if it is really only a handful of families you need, you might just
as well copy them into the peci headers and slap a comment above it
saying where they come from and save yourself all that churn...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2021-10-04 at 21:03 +0200, Borislav Petkov wrote:
On Tue, Aug 03, 2021 at 01:31:20PM +0200, Iwona Winiarska wrote:
quoted
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
Move the data from arch/x86/include/asm/intel-family.h into a new file
include/linux/x86/intel-family.h so that it can be used by other
architectures.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
To limit tree-wide changes and help people that were expecting
intel-family defines in arch/x86 to find it more easily without going
through git history, we're not removing the original header
completely, we're keeping it as a "stub" that includes the new one.
If there is a consensus that the tree-wide option is better,
we can choose this approach.
Why can't the linux/ namespace header include the x86 one so that
nothing changes for arch/x86/?
Same reason why PECI can't just include arch/x86 directly (we're building for
ARM, not x86).
And if it is really only a handful of families you need, you might just
as well copy them into the peci headers and slap a comment above it
saying where they come from and save yourself all that churn...
It's a handful of families for now - but I do expect the list to grow once new
platforms are introduced (and with that - duplicates have to be added in both
places).
Since the churn is relatively low I wanted to start with trying to keep things
clean first.
If you're against that - sure, we can duplicate.
Thanks
-Iwona
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Dave Hansen <hidden> Date: 2021-10-11 19:40:29
On 10/11/21 12:21 PM, Winiarska, Iwona wrote:
On Mon, 2021-10-04 at 21:03 +0200, Borislav Petkov wrote:
quoted
On Tue, Aug 03, 2021 at 01:31:20PM +0200, Iwona Winiarska wrote:
quoted
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
Move the data from arch/x86/include/asm/intel-family.h into a new file
include/linux/x86/intel-family.h so that it can be used by other
architectures.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
To limit tree-wide changes and help people that were expecting
intel-family defines in arch/x86 to find it more easily without going
through git history, we're not removing the original header
completely, we're keeping it as a "stub" that includes the new one.
If there is a consensus that the tree-wide option is better,
we can choose this approach.
Why can't the linux/ namespace header include the x86 one so that
nothing changes for arch/x86/?
Same reason why PECI can't just include arch/x86 directly (we're building for
ARM, not x86).
If you're in include/linux/x86-hacks.h, what prevents you from doing
#include "../../arch/x86/include/asm/intel-family.h"
?
In the end, to the compiler, it's just a file in a weird location in the
tree. I think I'd prefer one weird include to moving that file out of
arch/x86.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2021-10-11 at 12:40 -0700, Dave Hansen wrote:
On 10/11/21 12:21 PM, Winiarska, Iwona wrote:
quoted
On Mon, 2021-10-04 at 21:03 +0200, Borislav Petkov wrote:
quoted
On Tue, Aug 03, 2021 at 01:31:20PM +0200, Iwona Winiarska wrote:
quoted
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
Move the data from arch/x86/include/asm/intel-family.h into a new file
include/linux/x86/intel-family.h so that it can be used by other
architectures.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
To limit tree-wide changes and help people that were expecting
intel-family defines in arch/x86 to find it more easily without going
through git history, we're not removing the original header
completely, we're keeping it as a "stub" that includes the new one.
If there is a consensus that the tree-wide option is better,
we can choose this approach.
Why can't the linux/ namespace header include the x86 one so that
nothing changes for arch/x86/?
Same reason why PECI can't just include arch/x86 directly (we're building
for
ARM, not x86).
If you're in include/linux/x86-hacks.h, what prevents you from doing
#include "../../arch/x86/include/asm/intel-family.h"
?
In the end, to the compiler, it's just a file in a weird location in the
tree. I think I'd prefer one weird include to moving that file out of
arch/x86.
Using relative includes in include/linux is uncommon (I can see just one usage
in libfdt.h pulling stuff from scripts), so I thought I can't use it in this way
(seems slightly hacky to pull stuff from outside include path).
But if that would be ok, it looks like a good alternative to avoid duplication
in this case.
Thanks
-Iwona
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Dave Hansen <hidden> Date: 2021-10-11 23:12:27
On 10/11/21 1:53 PM, Winiarska, Iwona wrote:
quoted
If you're in include/linux/x86-hacks.h, what prevents you from doing
#include "../../arch/x86/include/asm/intel-family.h"
?
In the end, to the compiler, it's just a file in a weird location in the
tree. I think I'd prefer one weird include to moving that file out of
arch/x86.
Using relative includes in include/linux is uncommon (I can see just one usage
in libfdt.h pulling stuff from scripts), so I thought I can't use it in this way
(seems slightly hacky to pull stuff from outside include path).
But if that would be ok, it looks like a good alternative to avoid duplication
in this case.
If you don't want to do it from a header, you can also do it directly
from a .c file that's outside of arch/x86.
I think that's a much better alternative than moving stuff elsewhere.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Oct 11, 2021 at 07:21:26PM +0000, Winiarska, Iwona wrote:
Same reason why PECI can't just include arch/x86 directly (we're building for
ARM, not x86).
Aha.
So what do you need those INTEL_FAM6* defines for?
I see peci_cpu_device_ids[] which are used to match the CPU so at least
that thing must be loading on x86 hardware... reading your 0th message,
it sounds like that peci-cpu thing is loaded on an x86 CPU and it then
exposes those interfaces which a PECI controller accesses.
And then I see in init_core_mask() the single usage of INTEL_FAM6* and
that drivers/hwmon/peci/cputemp.c is a CPU temp monitoring client so
that thing probably runs on x86 too.
Or?
If it does, then you don't need the code move.
But it looks like I'm missing something...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2021-10-11 at 22:06 +0200, Borislav Petkov wrote:
On Mon, Oct 11, 2021 at 07:21:26PM +0000, Winiarska, Iwona wrote:
quoted
Same reason why PECI can't just include arch/x86 directly (we're building
for
ARM, not x86).
Aha.
So what do you need those INTEL_FAM6* defines for?
To identify the x86 CPU and use that as a match for binding PECI drivers.
I see peci_cpu_device_ids[] which are used to match the CPU so at least
that thing must be loading on x86 hardware... reading your 0th message,
it sounds like that peci-cpu thing is loaded on an x86 CPU and it then
exposes those interfaces which a PECI controller accesses.
Everything that's part of this series runs on the BMC (Baseboard Management
Controller). There's nothing ARM specific to it - it's just that the BMC
hardware we're currently supporting is ARM-based.
PECI is an interface that's exposed by some x86 CPUs - but that's a hardware
interface (available completely independent from whatever is actually running on
the x86 CPU).
And then I see in init_core_mask() the single usage of INTEL_FAM6* and
that drivers/hwmon/peci/cputemp.c is a CPU temp monitoring client so
that thing probably runs on x86 too.
That also runs on BMC, it uses functionality offered by peci-cpu to expose hwmon
interface to userspace.
Userspace that makes use of that hwmon interface also runs on the BMC and
exposes sensor data to user (via redfish API, or web-based interface).
Or?
If it does, then you don't need the code move.
But it looks like I'm missing something...
I'm sorry - it looks that my description in the cover letter wasn't clear
enough.
Thanks
-Iwona
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Oct 11, 2021 at 08:38:43PM +0000, Winiarska, Iwona wrote:
Everything that's part of this series runs on the BMC (Baseboard
Management Controller). There's nothing ARM specific to it - it's just
that the BMC hardware we're currently supporting is ARM-based. PECI is
an interface that's exposed by some x86 CPUs - but that's a hardware
interface (available completely independent from whatever is actually
running on the x86 CPU).
Aha, I think I got it: so this whole PECI pile is supposed to run on
the BMC - which can be ARM but doesn't have to be, i.e., code should be
generic enough - and the interfaces to the x86 CPU do get exposed to the
Linux running on the BMC.
Which brings me to the answer to your other mail:
On Mon, Oct 11, 2021 at 07:32:38PM +0000, Winiarska, Iwona wrote:
Nothing wrong - just a trade-off between churn and keeping things tidy
and not duplicated, similar to patch 1. And just like in patch 1, if
you have a strong opinion against it - we can duplicate.
So it is not about strong opinion. Rather, it is about whether this
exporting would be disadvantageous for x86 freedom. And I think it will
be:
Because if you exported those and then we went and changed those
interfaces and defines (changed their naming, function arguments,
whatever) and something outside of x86 used them, we will break that
something.
And usually we go and fix those users too but I doubt anyone has access
to that PECI hw to actually test fixes, etc, etc.
So I'd prefer the small amount of duplication vs external stuff using
x86 facilities any day of the week. And so I'd suggest you simply copy
the handful of functions and defines you're gonna be needing and the
defines and be done with it.
Dave's idea makes sense to me too but lately it keeps happening that
we change something in x86-land and it turns out something "from the
outside" is using it and it breaks, so it is a lot easier if things are
independent.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2021-10-11 at 23:31 +0200, Borislav Petkov wrote:
On Mon, Oct 11, 2021 at 08:38:43PM +0000, Winiarska, Iwona wrote:
quoted
Everything that's part of this series runs on the BMC (Baseboard
Management Controller). There's nothing ARM specific to it - it's just
that the BMC hardware we're currently supporting is ARM-based. PECI is
an interface that's exposed by some x86 CPUs - but that's a hardware
interface (available completely independent from whatever is actually
running on the x86 CPU).
Aha, I think I got it: so this whole PECI pile is supposed to run on
the BMC - which can be ARM but doesn't have to be, i.e., code should be
generic enough - and the interfaces to the x86 CPU do get exposed to the
Linux running on the BMC.
Which brings me to the answer to your other mail:
On Mon, Oct 11, 2021 at 07:32:38PM +0000, Winiarska, Iwona wrote:
quoted
Nothing wrong - just a trade-off between churn and keeping things tidy
and not duplicated, similar to patch 1. And just like in patch 1, if
you have a strong opinion against it - we can duplicate.
So it is not about strong opinion. Rather, it is about whether this
exporting would be disadvantageous for x86 freedom. And I think it will
be:
Because if you exported those and then we went and changed those
interfaces and defines (changed their naming, function arguments,
whatever) and something outside of x86 used them, we will break that
something.
And usually we go and fix those users too but I doubt anyone has access
to that PECI hw to actually test fixes, etc, etc.
We (OpenBMC) do have PECI HW, so that shouldn't be a problem.
So I'd prefer the small amount of duplication vs external stuff using
x86 facilities any day of the week. And so I'd suggest you simply copy
the handful of functions and defines you're gonna be needing and the
defines and be done with it.
Dave's idea makes sense to me too but lately it keeps happening that
we change something in x86-land and it turns out something "from the
outside" is using it and it breaks, so it is a lot easier if things are
independent.
Both CPUID.EAX=1 decoding and definitions in intel-family are pretty "well-
defined". I understand the scenario that you're describing, but in order to
break the outside user there would need to be some "logic" behind the pulled in
concepts (if, for example, I would use something like X86_MATCH_* defines in
PECI).
Thanks
-Iwona
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
The format of Package Identifier PCS register that describes CPUID
information has the same layout as CPUID_1.EAX, so let's allow to reuse
cpuid helpers by making it available for other architectures as well.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
MAINTAINERS | 1 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/cpu.h | 3 ---
arch/x86/include/asm/microcode.h | 2 +-
arch/x86/kvm/cpuid.h | 3 ++-
arch/x86/lib/Makefile | 2 +-
drivers/edac/mce_amd.c | 3 +--
include/linux/x86/cpu.h | 9 +++++++++
lib/Kconfig | 4 ++++
lib/Makefile | 2 ++
lib/x86/Makefile | 3 +++
{arch/x86/lib => lib/x86}/cpu.c | 2 +-
12 files changed, 26 insertions(+), 9 deletions(-)
create mode 100644 include/linux/x86/cpu.h
create mode 100644 lib/x86/Makefile
rename {arch/x86/lib => lib/x86}/cpu.c (95%)
diff --git a/arch/x86/lib/cpu.c b/lib/x86/cpu.csimilarity index 95%rename from arch/x86/lib/cpu.crename to lib/x86/cpu.cindex 7ad68917a51e..17af59a2fddf 100644--- a/arch/x86/lib/cpu.c+++ b/lib/x86/cpu.c
On Tue, Aug 03, 2021 at 01:31:21PM +0200, Iwona Winiarska wrote:
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
The format of Package Identifier PCS register that describes CPUID
information has the same layout as CPUID_1.EAX, so let's allow to reuse
cpuid helpers by making it available for other architectures as well.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
MAINTAINERS | 1 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/cpu.h | 3 ---
arch/x86/include/asm/microcode.h | 2 +-
arch/x86/kvm/cpuid.h | 3 ++-
arch/x86/lib/Makefile | 2 +-
drivers/edac/mce_amd.c | 3 +--
include/linux/x86/cpu.h | 9 +++++++++
lib/Kconfig | 4 ++++
lib/Makefile | 2 ++
lib/x86/Makefile | 3 +++
{arch/x86/lib => lib/x86}/cpu.c | 2 +-
12 files changed, 26 insertions(+), 9 deletions(-)
create mode 100644 include/linux/x86/cpu.h
create mode 100644 lib/x86/Makefile
rename {arch/x86/lib => lib/x86}/cpu.c (95%)
AFAICT, all that churn is done for x86_family() and x86_model() which
are used *exactly* *once* and which are almost trivial anyway.
What's wrong with simply computing the family and model "by hand", so to
speak, in peci_device_info_init() and do away with that diffstat
12 files changed, 26 insertions(+), 9 deletions(-)
?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2021-10-04 at 21:08 +0200, Borislav Petkov wrote:
On Tue, Aug 03, 2021 at 01:31:21PM +0200, Iwona Winiarska wrote:
quoted
Baseboard management controllers (BMC) often run Linux but are usually
implemented with non-X86 processors. They can use PECI to access package
config space (PCS) registers on the host CPU and since some information,
e.g. figuring out the core count, can be obtained using different
registers on different CPU generations, they need to decode the family
and model.
The format of Package Identifier PCS register that describes CPUID
information has the same layout as CPUID_1.EAX, so let's allow to reuse
cpuid helpers by making it available for other architectures as well.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Dan Williams <redacted>
---
MAINTAINERS | 1 +
arch/x86/Kconfig | 1 +
arch/x86/include/asm/cpu.h | 3 ---
arch/x86/include/asm/microcode.h | 2 +-
arch/x86/kvm/cpuid.h | 3 ++-
arch/x86/lib/Makefile | 2 +-
drivers/edac/mce_amd.c | 3 +--
include/linux/x86/cpu.h | 9 +++++++++
lib/Kconfig | 4 ++++
lib/Makefile | 2 ++
lib/x86/Makefile | 3 +++
{arch/x86/lib => lib/x86}/cpu.c | 2 +-
12 files changed, 26 insertions(+), 9 deletions(-)
create mode 100644 include/linux/x86/cpu.h
create mode 100644 lib/x86/Makefile
rename {arch/x86/lib => lib/x86}/cpu.c (95%)
AFAICT, all that churn is done for x86_family() and x86_model() which
are used *exactly* *once* and which are almost trivial anyway.
Correct.
What's wrong with simply computing the family and model "by hand", so to
speak, in peci_device_info_init() and do away with that diffstat
12 files changed, 26 insertions(+), 9 deletions(-)
?
Nothing wrong - just a trade-off between churn and keeping things tidy and not
duplicated, similar to patch 1.
And just like in patch 1, if you have a strong opinion against it - we can
duplicate.
Thanks
-Iwona
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -0,0 +1,33 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/peci/peci-controller.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Generic Device Tree Bindings for PECI++maintainers:+-Iwona Winiarska <iwona.winiarska@intel.com>++description:+PECI (Platform Environment Control Interface) is an interface that provides a+communication channel from Intel processors and chipset components to external+monitoring or control devices.++properties:+$nodename:+pattern:"^peci-controller(@.*)?$"++cmd-timeout-ms:+description:+Command timeout in units of ms.++additionalProperties:true++examples:+-|+peci-controller@1e78b000 {+reg = <0x1e78b000 0x100>;+cmd-timeout-ms = <500>;+};+...
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -0,0 +1,109 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/peci/peci-aspeed.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Aspeed PECI Bus Device Tree Bindings++maintainers:+-Iwona Winiarska <iwona.winiarska@intel.com>+-Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>++allOf:+-$ref:peci-controller.yaml#++properties:+compatible:+enum:+-aspeed,ast2400-peci+-aspeed,ast2500-peci+-aspeed,ast2600-peci++reg:+maxItems:1++interrupts:+maxItems:1++clocks:+description:+Clock source for PECI controller. Should reference the external+oscillator clock.+maxItems:1++resets:+maxItems:1++cmd-timeout-ms:+minimum:1+maximum:1000+default:1000++aspeed,clock-divider:+description:+This value determines PECI controller internal clock dividing+rate. The divider will be calculated as 2 raised to the power of+the given value.+$ref:/schemas/types.yaml#/definitions/uint32+minimum:0+maximum:7+default:0++aspeed,msg-timing:+description:+Message timing negotiation period. This value will determine the period+of message timing negotiation to be issued by PECI controller. The unit+of the programmed value is four times of PECI clock period.+$ref:/schemas/types.yaml#/definitions/uint32+minimum:0+maximum:255+default:1++aspeed,addr-timing:+description:+Address timing negotiation period. This value will determine the period+of address timing negotiation to be issued by PECI controller. The unit+of the programmed value is four times of PECI clock period.+$ref:/schemas/types.yaml#/definitions/uint32+minimum:0+maximum:255+default:1++aspeed,rd-sampling-point:+description:+Read sampling point selection. The whole period of a bit time will be+divided into 16 time frames. This value will determine the time frame+in which the controller will sample PECI signal for data read back.+Usually in the middle of a bit time is the best.+$ref:/schemas/types.yaml#/definitions/uint32+minimum:0+maximum:15+default:8++required:+-compatible+-reg+-interrupts+-clocks+-resets++additionalProperties:false++examples:+-|+#include <dt-bindings/interrupt-controller/arm-gic.h>+#include <dt-bindings/clock/ast2600-clock.h>+peci-controller@1e78b000 {+compatible = "aspeed,ast2600-peci";+reg = <0x1e78b000 0x100>;+interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;+clocks = <&syscon ASPEED_CLK_GATE_REF0CLK>;+resets = <&syscon ASPEED_RESET_PECI>;+cmd-timeout-ms = <1000>;+aspeed,clock-divider = <0>;+aspeed,msg-timing = <1>;+aspeed,addr-timing = <1>;+aspeed,rd-sampling-point = <8>;+};+...
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Intel processors provide access for various services designed to support
processor and DRAM thermal management, platform manageability and
processor interface tuning and diagnostics.
Those services are available via the Platform Environment Control
Interface (PECI) that provides a communication channel between the
processor and the Baseboard Management Controller (BMC) or other
platform management device.
This change introduces PECI subsystem by adding the initial core module
and API for controller drivers.
Co-developed-by: Jason M Bills <redacted>
Signed-off-by: Jason M Bills <redacted>
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 9 +++
drivers/Kconfig | 3 +
drivers/Makefile | 1 +
drivers/peci/Kconfig | 15 ++++
drivers/peci/Makefile | 5 ++
drivers/peci/core.c | 155 ++++++++++++++++++++++++++++++++++++++++
drivers/peci/internal.h | 16 +++++
include/linux/peci.h | 99 +++++++++++++++++++++++++
8 files changed, 303 insertions(+)
create mode 100644 drivers/peci/Kconfig
create mode 100644 drivers/peci/Makefile
create mode 100644 drivers/peci/core.c
create mode 100644 drivers/peci/internal.h
create mode 100644 include/linux/peci.h
From: Dan Williams <hidden> Date: 2021-08-25 22:59:10
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:
quoted hunk
Intel processors provide access for various services designed to support
processor and DRAM thermal management, platform manageability and
processor interface tuning and diagnostics.
Those services are available via the Platform Environment Control
Interface (PECI) that provides a communication channel between the
processor and the Baseboard Management Controller (BMC) or other
platform management device.
This change introduces PECI subsystem by adding the initial core module
and API for controller drivers.
Co-developed-by: Jason M Bills <redacted>
Signed-off-by: Jason M Bills <redacted>
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 9 +++
drivers/Kconfig | 3 +
drivers/Makefile | 1 +
drivers/peci/Kconfig | 15 ++++
drivers/peci/Makefile | 5 ++
drivers/peci/core.c | 155 ++++++++++++++++++++++++++++++++++++++++
drivers/peci/internal.h | 16 +++++
include/linux/peci.h | 99 +++++++++++++++++++++++++
8 files changed, 303 insertions(+)
create mode 100644 drivers/peci/Kconfig
create mode 100644 drivers/peci/Makefile
create mode 100644 drivers/peci/core.c
create mode 100644 drivers/peci/internal.h
create mode 100644 include/linux/peci.h
This seems late to be disabling power management, the device is about
to be freed. Keep in mind the lifetime of the this object can be
artificially prolonged. I expect this to be done when the device is
unregistered from the bus.
Shouldn't the get / put of this handle reference be bound to specific
accesses not held for the entire lifetime of the object? At a minimum
it seems to be a reference that can taken at registration and dropped
at unregistration.
Per above, are you sure unregistered devices need pm_runtime enabled?
Rest looks ok to me.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 2021-08-25 at 15:58 -0700, Dan Williams wrote:
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:
quoted
Intel processors provide access for various services designed to support
processor and DRAM thermal management, platform manageability and
processor interface tuning and diagnostics.
Those services are available via the Platform Environment Control
Interface (PECI) that provides a communication channel between the
processor and the Baseboard Management Controller (BMC) or other
platform management device.
This change introduces PECI subsystem by adding the initial core module
and API for controller drivers.
Co-developed-by: Jason M Bills <redacted>
Signed-off-by: Jason M Bills <redacted>
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 9 +++
drivers/Kconfig | 3 +
drivers/Makefile | 1 +
drivers/peci/Kconfig | 15 ++++
drivers/peci/Makefile | 5 ++
drivers/peci/core.c | 155 ++++++++++++++++++++++++++++++++++++++++
drivers/peci/internal.h | 16 +++++
include/linux/peci.h | 99 +++++++++++++++++++++++++
8 files changed, 303 insertions(+)
create mode 100644 drivers/peci/Kconfig
create mode 100644 drivers/peci/Makefile
create mode 100644 drivers/peci/core.c
create mode 100644 drivers/peci/internal.h
create mode 100644 include/linux/peci.h
This seems late to be disabling power management, the device is about
to be freed. Keep in mind the lifetime of the this object can be
artificially prolonged. I expect this to be done when the device is
unregistered from the bus.
Shouldn't the get / put of this handle reference be bound to specific
accesses not held for the entire lifetime of the object? At a minimum
it seems to be a reference that can taken at registration and dropped
at unregistration.
I'll move it to take ref at registration and to drop it at unregistration.
On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
reading through this buffer, but skip emitting it. Are you sure you
want to pay that overhead for every transaction?
spin_lock_irqsave() says "I don't know if interrupts are disabled
already, so I'll save the state, whatever it is, and restore later"
wait_for_completion_interruptible_timeout() says "I know I am in a
sleepable context where interrupts are enabled"
So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?
+ if (ret < 0)
+ return ret;
+
+ if (ret == 0) {
+ dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+ return -ETIMEDOUT;
+ }
+
+ spin_lock_irqsave(&priv->lock, flags);
+
+ writel(0, priv->base + ASPEED_PECI_CMD);
+
+ if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
+ spin_unlock_irqrestore(&priv->lock, flags);
+ dev_dbg(priv->dev, "No valid response!\n");
+ return -EIO;
+ }
+
+ spin_unlock_irqrestore(&priv->lock, flags);
+
+ memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, min_t(u8, req->rx.len, 16));
+ if (req->rx.len > 16)
+ memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_RD_DATA4,
+ req->rx.len - 16);
+
+ print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);
+
+ return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+ struct aspeed_peci *priv = arg;
+ u32 status;
+
+ spin_lock(&priv->lock);
+ status = readl(priv->base + ASPEED_PECI_INT_STS);
+ writel(status, priv->base + ASPEED_PECI_INT_STS);
+ priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+ /*
+ * In most cases, interrupt bits will be set one by one but also note
+ * that multiple interrupt bits could be set at the same time.
+ */
+ if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+ dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+ if (status & ASPEED_PECI_INT_BUS_CONTENTION)
+ dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONTENTION\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_BAD)
+ dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_WR_FCS_BAD\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
+ dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_WR_FCS_ABORT\n");
Are you sure these would not be better as tracepoints? If you're
debugging an interrupt related failure, the ratelimiting might get in
your way when you really need to know when one of these error
interrupts fire relative to another event.
+
+ /*
+ * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE bit
+ * set even in an error case.
+ */
+ if (status & ASPEED_PECI_INT_CMD_DONE)
+ complete(&priv->xfer_complete);
Hmm, no need to check if there was a sequencing error, like a command
was never submitted?
I'd recommend naming this devm_aspeed_peci_reset_control_deassert(),
because I came looking here from reading probe for why there was no
reassertion of reset on driver ->remove().
+{
+ int ret;
+
+ ret = reset_control_deassert(rst);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, aspeed_peci_reset_control_release, rst);
+}
+
+static void aspeed_peci_clk_release(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static int aspeed_peci_clk_enable(struct device *dev, struct clk *clk)
...ditto on the devm prefix, just to speed readability.
This support is also available as a module. If so, the module
will be called peci.
+
+if PECI
+
+source "drivers/peci/controller/Kconfig"
+
+endif # PECI
On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
reading through this buffer, but skip emitting it. Are you sure you
want to pay that overhead for every transaction?
I can remove it or I can add something like:
#if IS_ENABLED(CONFIG_PECI_DEBUG)
#define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__)
#else
#define peci_debug(...) do { } while (0)
#endif
(and similar peci_trace with trace_printk for usage in IRQ handlers and such).
What do you think?
spin_lock_irqsave() says "I don't know if interrupts are disabled
already, so I'll save the state, whatever it is, and restore later"
wait_for_completion_interruptible_timeout() says "I know I am in a
sleepable context where interrupts are enabled"
So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?
+
+ return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+ struct aspeed_peci *priv = arg;
+ u32 status;
+
+ spin_lock(&priv->lock);
+ status = readl(priv->base + ASPEED_PECI_INT_STS);
+ writel(status, priv->base + ASPEED_PECI_INT_STS);
+ priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+ /*
+ * In most cases, interrupt bits will be set one by one but also
note
+ * that multiple interrupt bits could be set at the same time.
+ */
+ if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+ if (status & ASPEED_PECI_INT_BUS_CONTENTION)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_BUS_CONTENTION\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_BAD)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_WR_FCS_BAD\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_WR_FCS_ABORT\n");
Are you sure these would not be better as tracepoints? If you're
debugging an interrupt related failure, the ratelimiting might get in
your way when you really need to know when one of these error
interrupts fire relative to another event.
Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status
would probably be too much.
I was thinking about something like trace_printk hidden under a
"CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future
improvement?
quoted
+
+ /*
+ * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE
bit
+ * set even in an error case.
+ */
+ if (status & ASPEED_PECI_INT_CMD_DONE)
+ complete(&priv->xfer_complete);
Hmm, no need to check if there was a sequencing error, like a command
was never submitted?
It's handled by checking if HW is idle in xfer before a command is sent, where
we just expect a single interrupt per command.
I'd recommend naming this devm_aspeed_peci_reset_control_deassert(),
because I came looking here from reading probe for why there was no
reassertion of reset on driver ->remove().
Ok.
quoted
+{
+ int ret;
+
+ ret = reset_control_deassert(rst);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev,
aspeed_peci_reset_control_release, rst);
+}
+
+static void aspeed_peci_clk_release(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static int aspeed_peci_clk_enable(struct device *dev, struct clk *clk)
...ditto on the devm prefix, just to speed readability.
On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
reading through this buffer, but skip emitting it. Are you sure you
want to pay that overhead for every transaction?
I can remove it or I can add something like:
#if IS_ENABLED(CONFIG_PECI_DEBUG)
#define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__)
#else
#define peci_debug(...) do { } while (0)
#endif
It's the hex dump I'm worried about, not the debug statements as much.
I think the choices are remove the print_hex_dump_bytes(), put it
behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is
skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead
if this is not a fast path / infrequently used.
(and similar peci_trace with trace_printk for usage in IRQ handlers and such).
What do you think?
In general, no, don't wrap the base level print routines with
driver-specific ones. Also, trace_printk() is only for debug builds.
Note that trace points are built to be even less overhead than
dev_dbg(), so there's no overhead concern with disabled tracepoints,
they literally translate to nops when disabled.
spin_lock_irqsave() says "I don't know if interrupts are disabled
already, so I'll save the state, whatever it is, and restore later"
wait_for_completion_interruptible_timeout() says "I know I am in a
sleepable context where interrupts are enabled"
So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?
+
+ return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+ struct aspeed_peci *priv = arg;
+ u32 status;
+
+ spin_lock(&priv->lock);
+ status = readl(priv->base + ASPEED_PECI_INT_STS);
+ writel(status, priv->base + ASPEED_PECI_INT_STS);
+ priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+ /*
+ * In most cases, interrupt bits will be set one by one but also
note
+ * that multiple interrupt bits could be set at the same time.
+ */
+ if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+ if (status & ASPEED_PECI_INT_BUS_CONTENTION)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_BUS_CONTENTION\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_BAD)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_WR_FCS_BAD\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_WR_FCS_ABORT\n");
Are you sure these would not be better as tracepoints? If you're
debugging an interrupt related failure, the ratelimiting might get in
your way when you really need to know when one of these error
interrupts fire relative to another event.
Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status
would probably be too much.
Tracepoints become ABI once someone ships tooling that depends on them
being there. These don't look attractive for a tool, and they don't
look difficult to maintain if the interrupt handler needs to be
reworked. I.e. it would be trivial to keep a dead tracepoint around if
worse came to worse to keep a tool from failing to load.
I was thinking about something like trace_printk hidden under a
"CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future
improvement?
Again trace_printk() is only for private builds.
quoted
quoted
+
+ /*
+ * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE
bit
+ * set even in an error case.
+ */
+ if (status & ASPEED_PECI_INT_CMD_DONE)
+ complete(&priv->xfer_complete);
Hmm, no need to check if there was a sequencing error, like a command
was never submitted?
It's handled by checking if HW is idle in xfer before a command is sent, where
we just expect a single interrupt per command.
I'm asking how do you determine if this status was spurious, or there
was a sequencing error in the driver?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of
reading through this buffer, but skip emitting it. Are you sure you
want to pay that overhead for every transaction?
I can remove it or I can add something like:
#if IS_ENABLED(CONFIG_PECI_DEBUG)
#define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__)
#else
#define peci_debug(...) do { } while (0)
#endif
It's the hex dump I'm worried about, not the debug statements as much.
I think the choices are remove the print_hex_dump_bytes(), put it
behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is
skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead
if this is not a fast path / infrequently used.
I will place it behind IS_ENABLED(CONFIG_DYNAMIC_DEBUG).
quoted
(and similar peci_trace with trace_printk for usage in IRQ handlers and
such).
What do you think?
In general, no, don't wrap the base level print routines with
driver-specific ones. Also, trace_printk() is only for debug builds.
Note that trace points are built to be even less overhead than
dev_dbg(), so there's no overhead concern with disabled tracepoints,
they literally translate to nops when disabled.
spin_lock_irqsave() says "I don't know if interrupts are disabled
already, so I'll save the state, whatever it is, and restore later"
wait_for_completion_interruptible_timeout() says "I know I am in a
sleepable context where interrupts are enabled"
So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()?
+
+ return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+ struct aspeed_peci *priv = arg;
+ u32 status;
+
+ spin_lock(&priv->lock);
+ status = readl(priv->base + ASPEED_PECI_INT_STS);
+ writel(status, priv->base + ASPEED_PECI_INT_STS);
+ priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+ /*
+ * In most cases, interrupt bits will be set one by one but also
note
+ * that multiple interrupt bits could be set at the same time.
+ */
+ if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+ if (status & ASPEED_PECI_INT_BUS_CONTENTION)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_BUS_CONTENTION\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_BAD)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_WR_FCS_BAD\n");
+
+ if (status & ASPEED_PECI_INT_WR_FCS_ABORT)
+ dev_dbg_ratelimited(priv->dev,
"ASPEED_PECI_INT_WR_FCS_ABORT\n");
Are you sure these would not be better as tracepoints? If you're
debugging an interrupt related failure, the ratelimiting might get in
your way when you really need to know when one of these error
interrupts fire relative to another event.
Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ
status
would probably be too much.
Tracepoints become ABI once someone ships tooling that depends on them
being there. These don't look attractive for a tool, and they don't
look difficult to maintain if the interrupt handler needs to be
reworked. I.e. it would be trivial to keep a dead tracepoint around if
worse came to worse to keep a tool from failing to load.
After more consideration, I would prefer to remove these logs for now - in case
of error I'll log full status in xfer().
quoted
I was thinking about something like trace_printk hidden under a
"CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future
improvement?
Again trace_printk() is only for private builds.
quoted
quoted
quoted
+
+ /*
+ * All commands should be ended up with a
ASPEED_PECI_INT_CMD_DONE
bit
+ * set even in an error case.
+ */
+ if (status & ASPEED_PECI_INT_CMD_DONE)
+ complete(&priv->xfer_complete);
Hmm, no need to check if there was a sequencing error, like a command
was never submitted?
It's handled by checking if HW is idle in xfer before a command is sent,
where
we just expect a single interrupt per command.
I'm asking how do you determine if this status was spurious, or there
was a sequencing error in the driver?
I don't think we have any means to determine it.
PECI itself doesn't provide any mechanism to verify it (there is no sequence
number or tag to match request/response).
We're relying on the fact that BMC is a requester and initiates communication
with CPU - the interrupt won't be generated if BMC doesn't send any request.
Thanks
-Iwona
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Since PECI devices are discoverable, we can dynamically detect devices
that are actually available in the system.
This change complements the earlier implementation by rescanning PECI
bus to detect available devices. For this purpose, it also introduces the
minimal API for PECI requests.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
drivers/peci/Makefile | 2 +-
drivers/peci/core.c | 33 ++++++++++++
drivers/peci/device.c | 114 ++++++++++++++++++++++++++++++++++++++++
drivers/peci/internal.h | 14 +++++
drivers/peci/request.c | 50 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 drivers/peci/device.c
create mode 100644 drivers/peci/request.c
From: Dan Williams <hidden> Date: 2021-08-27 19:01:24
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:
quoted hunk
Since PECI devices are discoverable, we can dynamically detect devices
that are actually available in the system.
This change complements the earlier implementation by rescanning PECI
bus to detect available devices. For this purpose, it also introduces the
minimal API for PECI requests.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
drivers/peci/Makefile | 2 +-
drivers/peci/core.c | 33 ++++++++++++
drivers/peci/device.c | 114 ++++++++++++++++++++++++++++++++++++++++
drivers/peci/internal.h | 14 +++++
drivers/peci/request.c | 50 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 drivers/peci/device.c
create mode 100644 drivers/peci/request.c
Seems a waste to do a heap allocation for this routine. Why not:
/*
* PECI Ping is a command encoded by tx_len = 0, rx_len = 0.
* We expect correct Write FCS if the device at the target address
* is able to respond.
*/
struct peci_request req = { 0 };
WARN_ON_ONCE() should only be here to help other kernel developers not
make this mistake However, another way to enforce this is to stop
exporting peci_request_alloc() and instead export helpers for specific
command types, and keep this detail internal to the core. If you keep
this, it needs a comment that it is only here to warn other
peci-client developers of their bug before it goes upstream.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, 2021-08-27 at 12:01 -0700, Dan Williams wrote:
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:
quoted
Since PECI devices are discoverable, we can dynamically detect devices
that are actually available in the system.
This change complements the earlier implementation by rescanning PECI
bus to detect available devices. For this purpose, it also introduces the
minimal API for PECI requests.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
drivers/peci/Makefile | 2 +-
drivers/peci/core.c | 33 ++++++++++++
drivers/peci/device.c | 114 ++++++++++++++++++++++++++++++++++++++++
drivers/peci/internal.h | 14 +++++
drivers/peci/request.c | 50 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 drivers/peci/device.c
create mode 100644 drivers/peci/request.c
Seems a waste to do a heap allocation for this routine. Why not:
/*
* PECI Ping is a command encoded by tx_len = 0, rx_len = 0.
* We expect correct Write FCS if the device at the target address
* is able to respond.
*/
struct peci_request req = { 0 };
WARN_ON_ONCE() should only be here to help other kernel developers not
make this mistake However, another way to enforce this is to stop
exporting peci_request_alloc() and instead export helpers for specific
command types, and keep this detail internal to the core. If you keep
this, it needs a comment that it is only here to warn other
peci-client developers of their bug before it goes upstream.
PECI devices may not be discoverable at the time when PECI controller is
being added (e.g. BMC can boot up when the Host system is still in S5).
Since we currently don't have the capabilities to figure out the Host
system state inside the PECI subsystem itself, we have to rely on
userspace to do it for us.
In the future, PECI subsystem may be expanded with mechanisms that allow
us to avoid depending on userspace interaction (e.g. CPU presence could
be detected using GPIO, and the information on whether it's discoverable
could be obtained over IPMI).
Unfortunately, those methods may ultimately not be available (support
will vary from platform to platform), which means that we still need
platform independent method triggered by userspace.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
---
Documentation/ABI/testing/sysfs-bus-peci | 16 +++++
drivers/peci/Makefile | 2 +-
drivers/peci/core.c | 3 +-
drivers/peci/device.c | 1 +
drivers/peci/internal.h | 5 ++
drivers/peci/sysfs.c | 82 ++++++++++++++++++++++++
6 files changed, 107 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-peci
create mode 100644 drivers/peci/sysfs.c
@@ -0,0 +1,16 @@+What: /sys/bus/peci/rescan+Date: July 2021+KernelVersion: 5.15+Contact: Iwona Winiarska <iwona.winiarska@intel.com>+Description:+ Writing a non-zero value to this attribute will+ initiate scan for PECI devices on all PECI controllers+ in the system.++What: /sys/bus/peci/devices/<controller_id>-<device_addr>/remove+Date: July 2021+KernelVersion: 5.15+Contact: Iwona Winiarska <iwona.winiarska@intel.com>+Description:+ Writing a non-zero value to this attribute will+ remove the PECI device and any of its children.
From: Dan Williams <hidden> Date: 2021-08-27 19:11:18
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:
PECI devices may not be discoverable at the time when PECI controller is
being added (e.g. BMC can boot up when the Host system is still in S5).
Since we currently don't have the capabilities to figure out the Host
system state inside the PECI subsystem itself, we have to rely on
userspace to do it for us.
In the future, PECI subsystem may be expanded with mechanisms that allow
us to avoid depending on userspace interaction (e.g. CPU presence could
be detected using GPIO, and the information on whether it's discoverable
could be obtained over IPMI).
Thanks for this detail.
quoted hunk
Unfortunately, those methods may ultimately not be available (support
will vary from platform to platform), which means that we still need
platform independent method triggered by userspace.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
---
Documentation/ABI/testing/sysfs-bus-peci | 16 +++++
drivers/peci/Makefile | 2 +-
drivers/peci/core.c | 3 +-
drivers/peci/device.c | 1 +
drivers/peci/internal.h | 5 ++
drivers/peci/sysfs.c | 82 ++++++++++++++++++++++++
6 files changed, 107 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-peci
create mode 100644 drivers/peci/sysfs.c
@@ -0,0 +1,16 @@+What: /sys/bus/peci/rescan+Date: July 2021+KernelVersion: 5.15+Contact: Iwona Winiarska <iwona.winiarska@intel.com>+Description:+ Writing a non-zero value to this attribute will+ initiate scan for PECI devices on all PECI controllers+ in the system.++What: /sys/bus/peci/devices/<controller_id>-<device_addr>/remove+Date: July 2021+KernelVersion: 5.15+Contact: Iwona Winiarska <iwona.winiarska@intel.com>+Description:+ Writing a non-zero value to this attribute will+ remove the PECI device and any of its children.
To me, sysfs.c is small enough to just fold into core.c, then no need
to declare public attribute arrays like this, but up to you if you
prefer the sysfs.c split.
How do you solve races between sysfs device remove and controller
device remove? Looks like double-free at first glance. Have a look at
the kill_device() helper as one way to resolve this double-delete
race..
On Fri, 2021-08-27 at 12:11 -0700, Dan Williams wrote:
On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska
[off-list ref] wrote:
quoted
PECI devices may not be discoverable at the time when PECI controller is
being added (e.g. BMC can boot up when the Host system is still in S5).
Since we currently don't have the capabilities to figure out the Host
system state inside the PECI subsystem itself, we have to rely on
userspace to do it for us.
In the future, PECI subsystem may be expanded with mechanisms that allow
us to avoid depending on userspace interaction (e.g. CPU presence could
be detected using GPIO, and the information on whether it's discoverable
could be obtained over IPMI).
Thanks for this detail.
quoted
Unfortunately, those methods may ultimately not be available (support
will vary from platform to platform), which means that we still need
platform independent method triggered by userspace.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
---
Documentation/ABI/testing/sysfs-bus-peci | 16 +++++
drivers/peci/Makefile | 2 +-
drivers/peci/core.c | 3 +-
drivers/peci/device.c | 1 +
drivers/peci/internal.h | 5 ++
drivers/peci/sysfs.c | 82 ++++++++++++++++++++++++
6 files changed, 107 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-peci
create mode 100644 drivers/peci/sysfs.c
@@ -0,0 +1,16 @@+What: /sys/bus/peci/rescan+Date: July 2021+KernelVersion: 5.15+Contact: Iwona Winiarska <iwona.winiarska@intel.com>+Description:+ Writing a non-zero value to this attribute will+ initiate scan for PECI devices on all PECI controllers+ in the system.++What: /sys/bus/peci/devices/<controller_id>-<device_addr>/remove+Date: July 2021+KernelVersion: 5.15+Contact: Iwona Winiarska <iwona.winiarska@intel.com>+Description:+ Writing a non-zero value to this attribute will+ remove the PECI device and any of its children.
To me, sysfs.c is small enough to just fold into core.c, then no need
to declare public attribute arrays like this, but up to you if you
prefer the sysfs.c split.
How do you solve races between sysfs device remove and controller
device remove? Looks like double-free at first glance. Have a look at
the kill_device() helper as one way to resolve this double-delete
race..
Here we're adding support for PECI device drivers, which unlike PECI
controller drivers are actually able to provide functionalities to
userspace.
We're also extending peci_request API to allow querying more details
about PECI device (e.g. model/family), that's going to be used to find
a compatible peci_driver.
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
drivers/peci/Kconfig | 1 +
drivers/peci/core.c | 49 +++++++++
drivers/peci/device.c | 105 ++++++++++++++++++++
drivers/peci/internal.h | 75 ++++++++++++++
drivers/peci/request.c | 214 ++++++++++++++++++++++++++++++++++++++++
include/linux/peci.h | 19 ++++
lib/Kconfig | 2 +-
7 files changed, 464 insertions(+), 1 deletion(-)
@@ -79,6 +153,10 @@ int peci_device_create(struct peci_controller *controller, u8 addr)device->dev.bus=&peci_bus_type;device->dev.type=&peci_device_type;+ret=peci_device_info_init(device);+if(ret)+gotoerr_free;+ret=dev_set_name(&device->dev,"%d-%02x",controller->id,device->addr);if(ret)gotoerr_free;
@@ -102,6 +180,33 @@ void peci_device_destroy(struct peci_device *device)device_unregister(&device->dev);}+int__peci_driver_register(structpeci_driver*driver,structmodule*owner,+constchar*mod_name)+{+driver->driver.bus=&peci_bus_type;+driver->driver.owner=owner;+driver->driver.mod_name=mod_name;++if(!driver->probe){+pr_err("peci: trying to register driver without probe callback\n");+return-EINVAL;+}++if(!driver->id_table){+pr_err("peci: trying to register driver without device id table\n");+return-EINVAL;+}++returndriver_register(&driver->driver);+}+EXPORT_SYMBOL_NS_GPL(__peci_driver_register,PECI);++voidpeci_driver_unregister(structpeci_driver*driver)+{+driver_unregister(&driver->driver);+}+EXPORT_SYMBOL_NS_GPL(peci_driver_unregister,PECI);+staticvoidpeci_device_release(structdevice*dev){structpeci_device*device=to_peci_device(dev);
GENERIC_LIB_X86 has dependencies, so this 'select' will make kbuild
unhappy when that dependency is not met. Given that this symbol
already selected by X86, it seems this just wants a "depends on
GENERIC_LIB_X86".
quoted hunk
help
The Platform Environment Control Interface (PECI) is an interface
that provides a communication channel to Intel processors and
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+
+ /*
+ * PECI device may be in a state where it is unable to return a proper
+ * DIB, in which case it returns 0 as DIB value.
+ * Let's treat this as an error to avoid carrying on with the detection
+ * using invalid revision.
+ */
+ dib = peci_request_data_dib(req);
I would expect peci_request_data_dib() to make a request.
A stack allocated peci_request passed to peci_get_dib() that returns
an error code would seem to be cleaner than this current organization.
This looks broken, what in the GENERIC_LIB_X86 implementation depends on peci?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
GENERIC_LIB_X86 has dependencies, so this 'select' will make kbuild
unhappy when that dependency is not met. Given that this symbol
already selected by X86, it seems this just wants a "depends on
GENERIC_LIB_X86".
Not applicable anymore after patches 1 and 2 got dropped following feedback from
arch/x86.
quoted
help
The Platform Environment Control Interface (PECI) is an interface
that provides a communication channel to Intel processors and
get_dib is a PECI command name - I changed the naming scheme slightly to make
things more clear, e.g. peci_xfer_get_dib().
quoted
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+
+ /*
+ * PECI device may be in a state where it is unable to return a
proper
+ * DIB, in which case it returns 0 as DIB value.
+ * Let's treat this as an error to avoid carrying on with the
detection
+ * using invalid revision.
+ */
+ dib = peci_request_data_dib(req);
I would expect peci_request_data_dib() to make a request.
Changed peci_request_data_dib to peci_request_dib_read and
peci_request_data_temp to peci_request_temp_read to align with
peci_request_data_read*.
A stack allocated peci_request passed to peci_get_dib() that returns
an error code would seem to be cleaner than this current organization.
*to_peci_controller(void *d)
* struct peci_device - PECI device
* @dev: device object to register PECI device to the device model
* @controller: manages the bus segment hosting this PECI device
+ * @info: PECI device characteristics
+ * @info.family: device family
+ * @info.model: device model
+ * @info.peci_revision: PECI revision supported by the PECI device
+ * @info.socket_id: the socket ID represented by the PECI device
* @addr: address used on the PECI bus connected to the parent controller
*
* A peci_device identifies a single device (i.e. CPU) connected to a PECI
bus.
config GENERIC_LIB_X86
bool
- depends on X86
+ depends on X86 || PECI
This looks broken, what in the GENERIC_LIB_X86 implementation depends on peci?
Not applicable anymore.
Apologies for the huge delay in my replies.
Thanks
-Iwona
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PECI is an interface that may be used by different types of devices.
Here we're adding a peci-cpu driver compatible with Intel processors.
The driver is responsible for handling auxiliary devices that can
subsequently be used by other drivers (e.g. hwmons).
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 1 +
drivers/peci/Kconfig | 15 ++
drivers/peci/Makefile | 2 +
drivers/peci/cpu.c | 344 +++++++++++++++++++++++++++++++++++++++
drivers/peci/device.c | 1 +
drivers/peci/internal.h | 27 +++
drivers/peci/request.c | 213 ++++++++++++++++++++++++
include/linux/peci-cpu.h | 38 +++++
include/linux/peci.h | 8 -
9 files changed, 641 insertions(+), 8 deletions(-)
create mode 100644 drivers/peci/cpu.c
create mode 100644 include/linux/peci-cpu.h
@@ -3,6 +3,8 @@# Core functionalitypeci-y:=core.orequest.odevice.osysfs.oobj-$(CONFIG_PECI)+=peci.o+peci-cpu-y:=cpu.o+obj-$(CONFIG_PECI_CPU)+=peci-cpu.o# Hardware specific bus driversobj-y+=controller/
Add peci-dimmtemp driver for Temperature Sensor on DIMM readings that
are accessible via the processor PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain thermal readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
Note that the timeout was completely removed - we're going to probe
for detected DIMMs every 5 seconds until we reach "stable" state of
either getting correct DIMM data or getting all -EINVAL (which
suggest that the CPU doesn't have any DIMMs).
drivers/hwmon/peci/Kconfig | 13 +
drivers/hwmon/peci/Makefile | 2 +
drivers/hwmon/peci/dimmtemp.c | 614 ++++++++++++++++++++++++++++++++++
3 files changed, 629 insertions(+)
create mode 100644 drivers/hwmon/peci/dimmtemp.c
@@ -0,0 +1,614 @@+// SPDX-License-Identifier: GPL-2.0-only+// Copyright (c) 2018-2021 Intel Corporation++#include<linux/auxiliary_bus.h>+#include<linux/bitfield.h>+#include<linux/bitops.h>+#include<linux/hwmon.h>+#include<linux/jiffies.h>+#include<linux/module.h>+#include<linux/peci.h>+#include<linux/peci-cpu.h>+#include<linux/units.h>+#include<linux/workqueue.h>+#include<linux/x86/intel-family.h>++#include"common.h"++#define DIMM_MASK_CHECK_DELAY_JIFFIES msecs_to_jiffies(5000)++/* Max number of channel ranks and DIMM index per channel */+#define CHAN_RANK_MAX_ON_HSX 8+#define DIMM_IDX_MAX_ON_HSX 3+#define CHAN_RANK_MAX_ON_BDX 4+#define DIMM_IDX_MAX_ON_BDX 3+#define CHAN_RANK_MAX_ON_BDXD 2+#define DIMM_IDX_MAX_ON_BDXD 2+#define CHAN_RANK_MAX_ON_SKX 6+#define DIMM_IDX_MAX_ON_SKX 2+#define CHAN_RANK_MAX_ON_ICX 8+#define DIMM_IDX_MAX_ON_ICX 2+#define CHAN_RANK_MAX_ON_ICXD 4+#define DIMM_IDX_MAX_ON_ICXD 2++#define CHAN_RANK_MAX CHAN_RANK_MAX_ON_HSX+#define DIMM_IDX_MAX DIMM_IDX_MAX_ON_HSX+#define DIMM_NUMS_MAX (CHAN_RANK_MAX * DIMM_IDX_MAX)++#define CPU_SEG_MASK GENMASK(23, 16)+#define GET_CPU_SEG(x) (((x) & CPU_SEG_MASK) >> 16)+#define CPU_BUS_MASK GENMASK(7, 0)+#define GET_CPU_BUS(x) ((x) & CPU_BUS_MASK)++#define DIMM_TEMP_MAX GENMASK(15, 8)+#define DIMM_TEMP_CRIT GENMASK(23, 16)+#define GET_TEMP_MAX(x) (((x) & DIMM_TEMP_MAX) >> 8)+#define GET_TEMP_CRIT(x) (((x) & DIMM_TEMP_CRIT) >> 16)++structpeci_dimmtemp;++structdimm_info{+intchan_rank_max;+intdimm_idx_max;+u8min_peci_revision;+int(*read_thresholds)(structpeci_dimmtemp*priv,intdimm_order,+intchan_rank,u32*data);+};++structpeci_dimm_thresholds{+longtemp_max;+longtemp_crit;+structpeci_sensor_statestate;+};++enumpeci_dimm_threshold_type{+temp_max_type,+temp_crit_type,+};++structpeci_dimmtemp{+structpeci_device*peci_dev;+structdevice*dev;+constchar*name;+conststructdimm_info*gen_info;+structdelayed_workdetect_work;+struct{+structpeci_sensor_datatemp;+structpeci_dimm_thresholdsthresholds;+}dimm[DIMM_NUMS_MAX];+char**dimmtemp_label;+DECLARE_BITMAP(dimm_mask,DIMM_NUMS_MAX);+};++staticu8__dimm_temp(u32reg,intdimm_order)+{+return(reg>>(dimm_order*8))&0xff;+}++staticintget_dimm_temp(structpeci_dimmtemp*priv,intdimm_no,long*val)+{+intdimm_order=dimm_no%priv->gen_info->dimm_idx_max;+intchan_rank=dimm_no/priv->gen_info->dimm_idx_max;+u32data;+intret;++mutex_lock(&priv->dimm[dimm_no].temp.state.lock);+if(!peci_sensor_need_update(&priv->dimm[dimm_no].temp.state))+gotoskip_update;++ret=peci_pcs_read(priv->peci_dev,PECI_PCS_DDR_DIMM_TEMP,chan_rank,&data);+if(ret){+mutex_unlock(&priv->dimm[dimm_no].temp.state.lock);+returnret;+}++priv->dimm[dimm_no].temp.value=__dimm_temp(data,dimm_order)*MILLIDEGREE_PER_DEGREE;++peci_sensor_mark_updated(&priv->dimm[dimm_no].temp.state);++skip_update:+*val=priv->dimm[dimm_no].temp.value;+mutex_unlock(&priv->dimm[dimm_no].temp.state.lock);+return0;+}++staticintupdate_thresholds(structpeci_dimmtemp*priv,intdimm_no)+{+intdimm_order=dimm_no%priv->gen_info->dimm_idx_max;+intchan_rank=dimm_no/priv->gen_info->dimm_idx_max;+u32data;+intret;++if(!peci_sensor_need_update(&priv->dimm[dimm_no].thresholds.state))+return0;++ret=priv->gen_info->read_thresholds(priv,dimm_order,chan_rank,&data);+if(ret==-ENODATA)/* Use default or previous value */+return0;+if(ret)+returnret;++priv->dimm[dimm_no].thresholds.temp_max=GET_TEMP_MAX(data)*MILLIDEGREE_PER_DEGREE;+priv->dimm[dimm_no].thresholds.temp_crit=GET_TEMP_CRIT(data)*MILLIDEGREE_PER_DEGREE;++peci_sensor_mark_updated(&priv->dimm[dimm_no].thresholds.state);++return0;+}++staticintget_dimm_thresholds(structpeci_dimmtemp*priv,enumpeci_dimm_threshold_typetype,+intdimm_no,long*val)+{+intret;++mutex_lock(&priv->dimm[dimm_no].thresholds.state.lock);+ret=update_thresholds(priv,dimm_no);+if(ret)+gotounlock;++switch(type){+casetemp_max_type:+*val=priv->dimm[dimm_no].thresholds.temp_max;+break;+casetemp_crit_type:+*val=priv->dimm[dimm_no].thresholds.temp_crit;+break;+default:+ret=-EOPNOTSUPP;+break;+}+unlock:+mutex_unlock(&priv->dimm[dimm_no].thresholds.state.lock);++returnret;+}++staticintdimmtemp_read_string(structdevice*dev,+enumhwmon_sensor_typestype,+u32attr,intchannel,constchar**str)+{+structpeci_dimmtemp*priv=dev_get_drvdata(dev);++if(attr!=hwmon_temp_label)+return-EOPNOTSUPP;++*str=(constchar*)priv->dimmtemp_label[channel];++return0;+}++staticintdimmtemp_read(structdevice*dev,enumhwmon_sensor_typestype,+u32attr,intchannel,long*val)+{+structpeci_dimmtemp*priv=dev_get_drvdata(dev);++switch(attr){+casehwmon_temp_input:+returnget_dimm_temp(priv,channel,val);+casehwmon_temp_max:+returnget_dimm_thresholds(priv,temp_max_type,channel,val);+casehwmon_temp_crit:+returnget_dimm_thresholds(priv,temp_crit_type,channel,val);+default:+break;+}++return-EOPNOTSUPP;+}++staticumode_tdimmtemp_is_visible(constvoid*data,enumhwmon_sensor_typestype,+u32attr,intchannel)+{+conststructpeci_dimmtemp*priv=data;++if(test_bit(channel,priv->dimm_mask))+return0444;++return0;+}++staticconststructhwmon_opspeci_dimmtemp_ops={+.is_visible=dimmtemp_is_visible,+.read_string=dimmtemp_read_string,+.read=dimmtemp_read,+};++staticintcheck_populated_dimms(structpeci_dimmtemp*priv)+{+intchan_rank_max=priv->gen_info->chan_rank_max;+intdimm_idx_max=priv->gen_info->dimm_idx_max;+u32chan_rank_empty=0;+u64dimm_mask=0;+intchan_rank,dimm_idx,ret;+u32pcs;++BUILD_BUG_ON(CHAN_RANK_MAX>32);+BUILD_BUG_ON(DIMM_NUMS_MAX>64);+if(chan_rank_max*dimm_idx_max>DIMM_NUMS_MAX){+WARN_ONCE(1,"Unsupported number of DIMMs");+return-EINVAL;+}++for(chan_rank=0;chan_rank<chan_rank_max;chan_rank++){+ret=peci_pcs_read(priv->peci_dev,PECI_PCS_DDR_DIMM_TEMP,chan_rank,&pcs);+if(ret){+/*+*Overall,weexpecteithersuccessor-EINVALin+*ordertodeterminewhetherDIMMispopulatedornot.+*Foranythingelse-wefallbacktodeferingthe+*detectiontobeperformedatalaterpointintime.+*/+if(ret==-EINVAL){+chan_rank_empty|=BIT(chan_rank);+continue;+}++return-EAGAIN;+}++for(dimm_idx=0;dimm_idx<dimm_idx_max;dimm_idx++)+if(__dimm_temp(pcs,dimm_idx))+dimm_mask|=BIT(chan_rank*dimm_idx_max+dimm_idx);+}++/* If we got all -EINVALs, it means that the CPU doesn't have any DIMMs. */+if(chan_rank_empty==GENMASK(chan_rank_max-1,0))+return-ENODEV;++/*+*It'spossiblethatmemorytrainingisnotdoneyet.Inthiscasewe+*deferthedetectiontobeperformedatalaterpointintime.+*/+if(!dimm_mask)+return-EAGAIN;++dev_dbg(priv->dev,"Scanned populated DIMMs: %#llx\n",dimm_mask);++bitmap_from_u64(priv->dimm_mask,dimm_mask);++return0;+}++staticintcreate_dimm_temp_label(structpeci_dimmtemp*priv,intchan)+{+intrank=chan/priv->gen_info->dimm_idx_max;+intidx=chan%priv->gen_info->dimm_idx_max;++priv->dimmtemp_label[chan]=devm_kasprintf(priv->dev,GFP_KERNEL,+"DIMM %c%d",'A'+rank,+idx+1);+if(!priv->dimmtemp_label[chan])+return-ENOMEM;++return0;+}++staticconstu32peci_dimmtemp_temp_channel_config[]={+[0...DIMM_NUMS_MAX-1]=HWMON_T_LABEL|HWMON_T_INPUT|HWMON_T_MAX|HWMON_T_CRIT,+0+};++staticconststructhwmon_channel_infopeci_dimmtemp_temp_channel={+.type=hwmon_temp,+.config=peci_dimmtemp_temp_channel_config,+};++staticconststructhwmon_channel_info*peci_dimmtemp_temp_info[]={+&peci_dimmtemp_temp_channel,+NULL+};++staticconststructhwmon_chip_infopeci_dimmtemp_chip_info={+.ops=&peci_dimmtemp_ops,+.info=peci_dimmtemp_temp_info,+};++staticintcreate_dimm_temp_info(structpeci_dimmtemp*priv)+{+intret,i,channels;+structdevice*dev;++/*+*WeexpecttoeitherfindpopulatedDIMMsandcarryonwithcreating+*sensors,orfindoutthattherearenoDIMMspopulated.+*Allotherstatesmeanthattheplatformneverreachedthestatethat+*allowstocheckDIMMstate-causingustoretrylateron.+*/+ret=check_populated_dimms(priv);+if(ret==-ENODEV){+dev_dbg(priv->dev,"No DIMMs found\n");+return0;+}elseif(ret){+schedule_delayed_work(&priv->detect_work,DIMM_MASK_CHECK_DELAY_JIFFIES);+dev_dbg(priv->dev,"Deferred populating DIMM temp info\n");+returnret;+}++channels=priv->gen_info->chan_rank_max*priv->gen_info->dimm_idx_max;++priv->dimmtemp_label=devm_kzalloc(priv->dev,channels*sizeof(char*),GFP_KERNEL);+if(!priv->dimmtemp_label)+return-ENOMEM;++for_each_set_bit(i,priv->dimm_mask,DIMM_NUMS_MAX){+ret=create_dimm_temp_label(priv,i);+if(ret)+returnret;+mutex_init(&priv->dimm[i].thresholds.state.lock);+mutex_init(&priv->dimm[i].temp.state.lock);+}++dev=devm_hwmon_device_register_with_info(priv->dev,priv->name,priv,+&peci_dimmtemp_chip_info,NULL);+if(IS_ERR(dev)){+dev_err(priv->dev,"Failed to register hwmon device\n");+returnPTR_ERR(dev);+}++dev_dbg(priv->dev,"%s: sensor '%s'\n",dev_name(dev),priv->name);++return0;+}++staticvoidcreate_dimm_temp_info_delayed(structwork_struct*work)+{+structpeci_dimmtemp*priv=container_of(to_delayed_work(work),+structpeci_dimmtemp,+detect_work);+intret;++ret=create_dimm_temp_info(priv);+if(ret&&ret!=-EAGAIN)+dev_err(priv->dev,"Failed to populate DIMM temp info\n");+}++staticvoidremove_delayed_work(void*_priv)+{+structpeci_dimmtemp*priv=_priv;++cancel_delayed_work_sync(&priv->detect_work);+}++staticintpeci_dimmtemp_probe(structauxiliary_device*adev,conststructauxiliary_device_id*id)+{+structdevice*dev=&adev->dev;+structpeci_device*peci_dev=to_peci_device(dev->parent);+structpeci_dimmtemp*priv;+intret;++priv=devm_kzalloc(dev,sizeof(*priv),GFP_KERNEL);+if(!priv)+return-ENOMEM;++priv->name=devm_kasprintf(dev,GFP_KERNEL,"peci_dimmtemp.cpu%d",+peci_dev->info.socket_id);+if(!priv->name)+return-ENOMEM;++priv->dev=dev;+priv->peci_dev=peci_dev;+priv->gen_info=(conststructdimm_info*)id->driver_data;++/*+*Thisisjustasanitycheck.Sincewe'reusingcommandsthatare+*guaranteedtobesupportedonagivenplatform,weshouldneversee+*revisionlowerthanexpected.+*/+if(peci_dev->info.peci_revision<priv->gen_info->min_peci_revision)+dev_warn(priv->dev,+"Unexpected PECI revision %#x, some features may be unavailable\n",+peci_dev->info.peci_revision);++INIT_DELAYED_WORK(&priv->detect_work,create_dimm_temp_info_delayed);++ret=devm_add_action_or_reset(priv->dev,remove_delayed_work,priv);+if(ret)+returnret;++ret=create_dimm_temp_info(priv);+if(ret&&ret!=-EAGAIN){+dev_err(dev,"Failed to populate DIMM temp info\n");+returnret;+}++return0;+}++staticint+read_thresholds_hsx(structpeci_dimmtemp*priv,intdimm_order,intchan_rank,u32*data)+{+u8dev,func;+u16reg;+intret;++/*+*Device20,Function0:IMC0channel0->rank0+*Device20,Function1:IMC0channel1->rank1+*Device21,Function0:IMC0channel2->rank2+*Device21,Function1:IMC0channel3->rank3+*Device23,Function0:IMC1channel0->rank4+*Device23,Function1:IMC1channel1->rank5+*Device24,Function0:IMC1channel2->rank6+*Device24,Function1:IMC1channel3->rank7+*/+dev=20+chan_rank/2+chan_rank/4;+func=chan_rank%2;+reg=0x120+dimm_order*4;++ret=peci_pci_local_read(priv->peci_dev,1,dev,func,reg,data);+if(ret)+returnret;++return0;+}++staticint+read_thresholds_bdxd(structpeci_dimmtemp*priv,intdimm_order,intchan_rank,u32*data)+{+u8dev,func;+u16reg;+intret;++/*+*Device10,Function2:IMC0channel0->rank0+*Device10,Function6:IMC0channel1->rank1+*Device12,Function2:IMC1channel0->rank2+*Device12,Function6:IMC1channel1->rank3+*/+dev=10+chan_rank/2*2;+func=(chan_rank%2)?6:2;+reg=0x120+dimm_order*4;++ret=peci_pci_local_read(priv->peci_dev,2,dev,func,reg,data);+if(ret)+returnret;++return0;+}++staticint+read_thresholds_skx(structpeci_dimmtemp*priv,intdimm_order,intchan_rank,u32*data)+{+u8dev,func;+u16reg;+intret;++/*+*Device10,Function2:IMC0channel0->rank0+*Device10,Function6:IMC0channel1->rank1+*Device11,Function2:IMC0channel2->rank2+*Device12,Function2:IMC1channel0->rank3+*Device12,Function6:IMC1channel1->rank4+*Device13,Function2:IMC1channel2->rank5+*/+dev=10+chan_rank/3*2+(chan_rank%3==2?1:0);+func=chan_rank%3==1?6:2;+reg=0x120+dimm_order*4;++ret=peci_pci_local_read(priv->peci_dev,2,dev,func,reg,data);+if(ret)+returnret;++return0;+}++staticint+read_thresholds_icx(structpeci_dimmtemp*priv,intdimm_order,intchan_rank,u32*data)+{+u32reg_val;+u64offset;+intret;+u8dev;++ret=peci_ep_pci_local_read(priv->peci_dev,0,13,0,2,0xd4,®_val);+if(ret||!(reg_val&BIT(31)))+return-ENODATA;/* Use default or previous value */++ret=peci_ep_pci_local_read(priv->peci_dev,0,13,0,2,0xd0,®_val);+if(ret)+return-ENODATA;/* Use default or previous value */++/*+*Device26,Offset224e0:IMC0channel0->rank0+*Device26,Offset264e0:IMC0channel1->rank1+*Device27,Offset224e0:IMC1channel0->rank2+*Device27,Offset264e0:IMC1channel1->rank3+*Device28,Offset224e0:IMC2channel0->rank4+*Device28,Offset264e0:IMC2channel1->rank5+*Device29,Offset224e0:IMC3channel0->rank6+*Device29,Offset264e0:IMC3channel1->rank7+*/+dev=26+chan_rank/2;+offset=0x224e0+dimm_order*4+(chan_rank%2)*0x4000;++ret=peci_mmio_read(priv->peci_dev,0,GET_CPU_SEG(reg_val),GET_CPU_BUS(reg_val),+dev,0,offset,data);+if(ret)+returnret;++return0;+}++staticconststructdimm_infodimm_hsx={+.chan_rank_max=CHAN_RANK_MAX_ON_HSX,+.dimm_idx_max=DIMM_IDX_MAX_ON_HSX,+.min_peci_revision=0x33,+.read_thresholds=&read_thresholds_hsx,+};++staticconststructdimm_infodimm_bdx={+.chan_rank_max=CHAN_RANK_MAX_ON_BDX,+.dimm_idx_max=DIMM_IDX_MAX_ON_BDX,+.min_peci_revision=0x33,+.read_thresholds=&read_thresholds_hsx,+};++staticconststructdimm_infodimm_bdxd={+.chan_rank_max=CHAN_RANK_MAX_ON_BDXD,+.dimm_idx_max=DIMM_IDX_MAX_ON_BDXD,+.min_peci_revision=0x33,+.read_thresholds=&read_thresholds_bdxd,+};++staticconststructdimm_infodimm_skx={+.chan_rank_max=CHAN_RANK_MAX_ON_SKX,+.dimm_idx_max=DIMM_IDX_MAX_ON_SKX,+.min_peci_revision=0x33,+.read_thresholds=&read_thresholds_skx,+};++staticconststructdimm_infodimm_icx={+.chan_rank_max=CHAN_RANK_MAX_ON_ICX,+.dimm_idx_max=DIMM_IDX_MAX_ON_ICX,+.min_peci_revision=0x40,+.read_thresholds=&read_thresholds_icx,+};++staticconststructdimm_infodimm_icxd={+.chan_rank_max=CHAN_RANK_MAX_ON_ICXD,+.dimm_idx_max=DIMM_IDX_MAX_ON_ICXD,+.min_peci_revision=0x40,+.read_thresholds=&read_thresholds_icx,+};++staticconststructauxiliary_device_idpeci_dimmtemp_ids[]={+{+.name="peci_cpu.dimmtemp.hsx",+.driver_data=(kernel_ulong_t)&dimm_hsx,+},+{+.name="peci_cpu.dimmtemp.bdx",+.driver_data=(kernel_ulong_t)&dimm_bdx,+},+{+.name="peci_cpu.dimmtemp.bdxd",+.driver_data=(kernel_ulong_t)&dimm_bdxd,+},+{+.name="peci_cpu.dimmtemp.skx",+.driver_data=(kernel_ulong_t)&dimm_skx,+},+{+.name="peci_cpu.dimmtemp.icx",+.driver_data=(kernel_ulong_t)&dimm_icx,+},+{+.name="peci_cpu.dimmtemp.icxd",+.driver_data=(kernel_ulong_t)&dimm_icxd,+},+{}+};+MODULE_DEVICE_TABLE(auxiliary,peci_dimmtemp_ids);++staticstructauxiliary_driverpeci_dimmtemp_driver={+.probe=peci_dimmtemp_probe,+.id_table=peci_dimmtemp_ids,+};++module_auxiliary_driver(peci_dimmtemp_driver);++MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");+MODULE_AUTHOR("Iwona Winiarska <iwona.winiarska@intel.com>");+MODULE_DESCRIPTION("PECI dimmtemp driver");+MODULE_LICENSE("GPL");+MODULE_IMPORT_NS(PECI_CPU);
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 03, 2021 at 01:31:32PM +0200, Iwona Winiarska wrote:
quoted hunk
Add peci-dimmtemp driver for Temperature Sensor on DIMM readings that
are accessible via the processor PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain thermal readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
Note that the timeout was completely removed - we're going to probe
for detected DIMMs every 5 seconds until we reach "stable" state of
either getting correct DIMM data or getting all -EINVAL (which
suggest that the CPU doesn't have any DIMMs).
drivers/hwmon/peci/Kconfig | 13 +
drivers/hwmon/peci/Makefile | 2 +
drivers/hwmon/peci/dimmtemp.c | 614 ++++++++++++++++++++++++++++++++++
3 files changed, 629 insertions(+)
create mode 100644 drivers/hwmon/peci/dimmtemp.c
I don't immediately see the value of those build bugs. What happens if
CHAN_RANK_MAX > 32 or DIMM_NUMS_MAX > 64 ? Where do those limits come
from ?
+ if (chan_rank_max * dimm_idx_max > DIMM_NUMS_MAX) {
+ WARN_ONCE(1, "Unsupported number of DIMMs");
Maybe display the values (chan_rank_max and dimm_idx_max).
+ return -EINVAL;
+ }
+
+ for (chan_rank = 0; chan_rank < chan_rank_max; chan_rank++) {
+ ret = peci_pcs_read(priv->peci_dev, PECI_PCS_DDR_DIMM_TEMP, chan_rank, &pcs);
+ if (ret) {
+ /*
+ * Overall, we expect either success or -EINVAL in
+ * order to determine whether DIMM is populated or not.
+ * For anything else - we fall back to defering the
Why " - " ?
+ * detection to be performed at a later point in time.
+ */
+ if (ret == -EINVAL) {
+ chan_rank_empty |= BIT(chan_rank);
+ continue;
+ }
+
+ return -EAGAIN;
+ }
+
+ for (dimm_idx = 0; dimm_idx < dimm_idx_max; dimm_idx++)
+ if (__dimm_temp(pcs, dimm_idx))
+ dimm_mask |= BIT(chan_rank * dimm_idx_max + dimm_idx);
+ }
+
+ /* If we got all -EINVALs, it means that the CPU doesn't have any DIMMs. */
+ if (chan_rank_empty == GENMASK(chan_rank_max - 1, 0))
+ return -ENODEV;
+
+ /*
+ * It's possible that memory training is not done yet. In this case we
+ * defer the detection to be performed at a later point in time.
+ */
+ if (!dimm_mask)
+ return -EAGAIN;
+
+ dev_dbg(priv->dev, "Scanned populated DIMMs: %#llx\n", dimm_mask);
+
+ bitmap_from_u64(priv->dimm_mask, dimm_mask);
+
+ return 0;
+}
+
+static int create_dimm_temp_label(struct peci_dimmtemp *priv, int chan)
+{
+ int rank = chan / priv->gen_info->dimm_idx_max;
+ int idx = chan % priv->gen_info->dimm_idx_max;
+
+ priv->dimmtemp_label[chan] = devm_kasprintf(priv->dev, GFP_KERNEL,
+ "DIMM %c%d", 'A' + rank,
+ idx + 1);
+ if (!priv->dimmtemp_label[chan])
+ return -ENOMEM;
+
+ return 0;
+}
+
+static const u32 peci_dimmtemp_temp_channel_config[] = {
+ [0 ... DIMM_NUMS_MAX - 1] = HWMON_T_LABEL | HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT,
+ 0
+};
+
+static const struct hwmon_channel_info peci_dimmtemp_temp_channel = {
+ .type = hwmon_temp,
+ .config = peci_dimmtemp_temp_channel_config,
+};
+
+static const struct hwmon_channel_info *peci_dimmtemp_temp_info[] = {
+ &peci_dimmtemp_temp_channel,
+ NULL
+};
+
+static const struct hwmon_chip_info peci_dimmtemp_chip_info = {
+ .ops = &peci_dimmtemp_ops,
+ .info = peci_dimmtemp_temp_info,
+};
+
+static int create_dimm_temp_info(struct peci_dimmtemp *priv)
+{
+ int ret, i, channels;
+ struct device *dev;
+
+ /*
+ * We expect to either find populated DIMMs and carry on with creating
+ * sensors, or find out that there are no DIMMs populated.
+ * All other states mean that the platform never reached the state that
+ * allows to check DIMM state - causing us to retry later on.
+ */
+ ret = check_populated_dimms(priv);
+ if (ret == -ENODEV) {
+ dev_dbg(priv->dev, "No DIMMs found\n");
+ return 0;
+ } else if (ret) {
+ schedule_delayed_work(&priv->detect_work, DIMM_MASK_CHECK_DELAY_JIFFIES);
+ dev_dbg(priv->dev, "Deferred populating DIMM temp info\n");
+ return ret;
+ }
+
+ channels = priv->gen_info->chan_rank_max * priv->gen_info->dimm_idx_max;
+
+ priv->dimmtemp_label = devm_kzalloc(priv->dev, channels * sizeof(char *), GFP_KERNEL);
+ if (!priv->dimmtemp_label)
+ return -ENOMEM;
+
+ for_each_set_bit(i, priv->dimm_mask, DIMM_NUMS_MAX) {
+ ret = create_dimm_temp_label(priv, i);
+ if (ret)
+ return ret;
+ mutex_init(&priv->dimm[i].thresholds.state.lock);
+ mutex_init(&priv->dimm[i].temp.state.lock);
+ }
+
+ dev = devm_hwmon_device_register_with_info(priv->dev, priv->name, priv,
+ &peci_dimmtemp_chip_info, NULL);
+ if (IS_ERR(dev)) {
+ dev_err(priv->dev, "Failed to register hwmon device\n");
+ return PTR_ERR(dev);
+ }
+
+ dev_dbg(priv->dev, "%s: sensor '%s'\n", dev_name(dev), priv->name);
+
+ return 0;
+}
+
+static void create_dimm_temp_info_delayed(struct work_struct *work)
+{
+ struct peci_dimmtemp *priv = container_of(to_delayed_work(work),
+ struct peci_dimmtemp,
+ detect_work);
+ int ret;
+
+ ret = create_dimm_temp_info(priv);
+ if (ret && ret != -EAGAIN)
+ dev_err(priv->dev, "Failed to populate DIMM temp info\n");
+}
+
+static void remove_delayed_work(void *_priv)
+{
+ struct peci_dimmtemp *priv = _priv;
+
+ cancel_delayed_work_sync(&priv->detect_work);
+}
+
+static int peci_dimmtemp_probe(struct auxiliary_device *adev, const struct auxiliary_device_id *id)
+{
+ struct device *dev = &adev->dev;
+ struct peci_device *peci_dev = to_peci_device(dev->parent);
+ struct peci_dimmtemp *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->name = devm_kasprintf(dev, GFP_KERNEL, "peci_dimmtemp.cpu%d",
+ peci_dev->info.socket_id);
+ if (!priv->name)
+ return -ENOMEM;
+
+ priv->dev = dev;
+ priv->peci_dev = peci_dev;
+ priv->gen_info = (const struct dimm_info *)id->driver_data;
+
+ /*
+ * This is just a sanity check. Since we're using commands that are
+ * guaranteed to be supported on a given platform, we should never see
+ * revision lower than expected.
+ */
+ if (peci_dev->info.peci_revision < priv->gen_info->min_peci_revision)
+ dev_warn(priv->dev,
+ "Unexpected PECI revision %#x, some features may be unavailable\n",
+ peci_dev->info.peci_revision);
+
+ INIT_DELAYED_WORK(&priv->detect_work, create_dimm_temp_info_delayed);
+
+ ret = devm_add_action_or_reset(priv->dev, remove_delayed_work, priv);
+ if (ret)
+ return ret;
+
+ ret = create_dimm_temp_info(priv);
+ if (ret && ret != -EAGAIN) {
+ dev_err(dev, "Failed to populate DIMM temp info\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int
+read_thresholds_hsx(struct peci_dimmtemp *priv, int dimm_order, int chan_rank, u32 *data)
+{
+ u8 dev, func;
+ u16 reg;
+ int ret;
+
+ /*
+ * Device 20, Function 0: IMC 0 channel 0 -> rank 0
+ * Device 20, Function 1: IMC 0 channel 1 -> rank 1
+ * Device 21, Function 0: IMC 0 channel 2 -> rank 2
+ * Device 21, Function 1: IMC 0 channel 3 -> rank 3
+ * Device 23, Function 0: IMC 1 channel 0 -> rank 4
+ * Device 23, Function 1: IMC 1 channel 1 -> rank 5
+ * Device 24, Function 0: IMC 1 channel 2 -> rank 6
+ * Device 24, Function 1: IMC 1 channel 3 -> rank 7
+ */
+ dev = 20 + chan_rank / 2 + chan_rank / 4;
+ func = chan_rank % 2;
+ reg = 0x120 + dimm_order * 4;
+
+ ret = peci_pci_local_read(priv->peci_dev, 1, dev, func, reg, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int
+read_thresholds_bdxd(struct peci_dimmtemp *priv, int dimm_order, int chan_rank, u32 *data)
+{
+ u8 dev, func;
+ u16 reg;
+ int ret;
+
+ /*
+ * Device 10, Function 2: IMC 0 channel 0 -> rank 0
+ * Device 10, Function 6: IMC 0 channel 1 -> rank 1
+ * Device 12, Function 2: IMC 1 channel 0 -> rank 2
+ * Device 12, Function 6: IMC 1 channel 1 -> rank 3
+ */
+ dev = 10 + chan_rank / 2 * 2;
+ func = (chan_rank % 2) ? 6 : 2;
+ reg = 0x120 + dimm_order * 4;
+
+ ret = peci_pci_local_read(priv->peci_dev, 2, dev, func, reg, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int
+read_thresholds_skx(struct peci_dimmtemp *priv, int dimm_order, int chan_rank, u32 *data)
+{
+ u8 dev, func;
+ u16 reg;
+ int ret;
+
+ /*
+ * Device 10, Function 2: IMC 0 channel 0 -> rank 0
+ * Device 10, Function 6: IMC 0 channel 1 -> rank 1
+ * Device 11, Function 2: IMC 0 channel 2 -> rank 2
+ * Device 12, Function 2: IMC 1 channel 0 -> rank 3
+ * Device 12, Function 6: IMC 1 channel 1 -> rank 4
+ * Device 13, Function 2: IMC 1 channel 2 -> rank 5
+ */
+ dev = 10 + chan_rank / 3 * 2 + (chan_rank % 3 == 2 ? 1 : 0);
+ func = chan_rank % 3 == 1 ? 6 : 2;
+ reg = 0x120 + dimm_order * 4;
+
+ ret = peci_pci_local_read(priv->peci_dev, 2, dev, func, reg, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int
+read_thresholds_icx(struct peci_dimmtemp *priv, int dimm_order, int chan_rank, u32 *data)
+{
+ u32 reg_val;
+ u64 offset;
+ int ret;
+ u8 dev;
+
+ ret = peci_ep_pci_local_read(priv->peci_dev, 0, 13, 0, 2, 0xd4, ®_val);
+ if (ret || !(reg_val & BIT(31)))
+ return -ENODATA; /* Use default or previous value */
+
+ ret = peci_ep_pci_local_read(priv->peci_dev, 0, 13, 0, 2, 0xd0, ®_val);
+ if (ret)
+ return -ENODATA; /* Use default or previous value */
+
+ /*
+ * Device 26, Offset 224e0: IMC 0 channel 0 -> rank 0
+ * Device 26, Offset 264e0: IMC 0 channel 1 -> rank 1
+ * Device 27, Offset 224e0: IMC 1 channel 0 -> rank 2
+ * Device 27, Offset 264e0: IMC 1 channel 1 -> rank 3
+ * Device 28, Offset 224e0: IMC 2 channel 0 -> rank 4
+ * Device 28, Offset 264e0: IMC 2 channel 1 -> rank 5
+ * Device 29, Offset 224e0: IMC 3 channel 0 -> rank 6
+ * Device 29, Offset 264e0: IMC 3 channel 1 -> rank 7
+ */
+ dev = 26 + chan_rank / 2;
+ offset = 0x224e0 + dimm_order * 4 + (chan_rank % 2) * 0x4000;
+
+ ret = peci_mmio_read(priv->peci_dev, 0, GET_CPU_SEG(reg_val), GET_CPU_BUS(reg_val),
+ dev, 0, offset, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct dimm_info dimm_hsx = {
+ .chan_rank_max = CHAN_RANK_MAX_ON_HSX,
+ .dimm_idx_max = DIMM_IDX_MAX_ON_HSX,
+ .min_peci_revision = 0x33,
+ .read_thresholds = &read_thresholds_hsx,
+};
+
+static const struct dimm_info dimm_bdx = {
+ .chan_rank_max = CHAN_RANK_MAX_ON_BDX,
+ .dimm_idx_max = DIMM_IDX_MAX_ON_BDX,
+ .min_peci_revision = 0x33,
+ .read_thresholds = &read_thresholds_hsx,
+};
+
+static const struct dimm_info dimm_bdxd = {
+ .chan_rank_max = CHAN_RANK_MAX_ON_BDXD,
+ .dimm_idx_max = DIMM_IDX_MAX_ON_BDXD,
+ .min_peci_revision = 0x33,
+ .read_thresholds = &read_thresholds_bdxd,
+};
+
+static const struct dimm_info dimm_skx = {
+ .chan_rank_max = CHAN_RANK_MAX_ON_SKX,
+ .dimm_idx_max = DIMM_IDX_MAX_ON_SKX,
+ .min_peci_revision = 0x33,
+ .read_thresholds = &read_thresholds_skx,
+};
+
+static const struct dimm_info dimm_icx = {
+ .chan_rank_max = CHAN_RANK_MAX_ON_ICX,
+ .dimm_idx_max = DIMM_IDX_MAX_ON_ICX,
+ .min_peci_revision = 0x40,
+ .read_thresholds = &read_thresholds_icx,
+};
+
+static const struct dimm_info dimm_icxd = {
+ .chan_rank_max = CHAN_RANK_MAX_ON_ICXD,
+ .dimm_idx_max = DIMM_IDX_MAX_ON_ICXD,
+ .min_peci_revision = 0x40,
+ .read_thresholds = &read_thresholds_icx,
+};
+
+static const struct auxiliary_device_id peci_dimmtemp_ids[] = {
+ {
+ .name = "peci_cpu.dimmtemp.hsx",
+ .driver_data = (kernel_ulong_t)&dimm_hsx,
+ },
+ {
+ .name = "peci_cpu.dimmtemp.bdx",
+ .driver_data = (kernel_ulong_t)&dimm_bdx,
+ },
+ {
+ .name = "peci_cpu.dimmtemp.bdxd",
+ .driver_data = (kernel_ulong_t)&dimm_bdxd,
+ },
+ {
+ .name = "peci_cpu.dimmtemp.skx",
+ .driver_data = (kernel_ulong_t)&dimm_skx,
+ },
+ {
+ .name = "peci_cpu.dimmtemp.icx",
+ .driver_data = (kernel_ulong_t)&dimm_icx,
+ },
+ {
+ .name = "peci_cpu.dimmtemp.icxd",
+ .driver_data = (kernel_ulong_t)&dimm_icxd,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(auxiliary, peci_dimmtemp_ids);
+
+static struct auxiliary_driver peci_dimmtemp_driver = {
+ .probe = peci_dimmtemp_probe,
+ .id_table = peci_dimmtemp_ids,
+};
+
+module_auxiliary_driver(peci_dimmtemp_driver);
+
+MODULE_AUTHOR("Jae Hyun Yoo [off-list ref]");
+MODULE_AUTHOR("Iwona Winiarska [off-list ref]");
+MODULE_DESCRIPTION("PECI dimmtemp driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PECI_CPU);
On Tue, 2021-08-03 at 08:39 -0700, Guenter Roeck wrote:
On Tue, Aug 03, 2021 at 01:31:32PM +0200, Iwona Winiarska wrote:
quoted
Add peci-dimmtemp driver for Temperature Sensor on DIMM readings that
are accessible via the processor PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain thermal readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
Note that the timeout was completely removed - we're going to probe
for detected DIMMs every 5 seconds until we reach "stable" state of
either getting correct DIMM data or getting all -EINVAL (which
suggest that the CPU doesn't have any DIMMs).
drivers/hwmon/peci/Kconfig | 13 +
drivers/hwmon/peci/Makefile | 2 +
drivers/hwmon/peci/dimmtemp.c | 614 ++++++++++++++++++++++++++++++++++
3 files changed, 629 insertions(+)
create mode 100644 drivers/hwmon/peci/dimmtemp.c
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 client"
+ depends on PECI
+ select SENSORS_PECI
+ select PECI_CPU
+ help
+ If you say yes here you get support for the generic Intel PECI
hwmon
+ driver which provides Temperature Sensor on DIMM readings that are
+ accessible via the processor PECI interface.
+
+ This driver can also be built as a module. If so, the module
+ will be called peci-dimmtemp.
+
config SENSORS_PECI
tristate
I don't immediately see the value of those build bugs. What happens if
CHAN_RANK_MAX > 32 or DIMM_NUMS_MAX > 64 ? Where do those limits come
from ?
Supported HW doesn't come near the limit for now - it's just an "artificial"
limit imposed by variables we're using (u64 for dimm_mask and u32 for
chan_rank_empty).
quoted
+ if (chan_rank_max * dimm_idx_max > DIMM_NUMS_MAX) {
+ WARN_ONCE(1, "Unsupported number of DIMMs");
Maybe display the values (chan_rank_max and dimm_idx_max).
Ok.
quoted
+ return -EINVAL;
+ }
+
+ for (chan_rank = 0; chan_rank < chan_rank_max; chan_rank++) {
+ ret = peci_pcs_read(priv->peci_dev, PECI_PCS_DDR_DIMM_TEMP,
chan_rank, &pcs);
+ if (ret) {
+ /*
+ * Overall, we expect either success or -EINVAL in
+ * order to determine whether DIMM is populated or
not.
+ * For anything else - we fall back to defering the
Why " - " ?
Hum... No idea after reading it now.
I'll drop it.
Thank you
-Iwona
quoted
+ * detection to be performed at a later point in
time.
+ */
+ if (ret == -EINVAL) {
+ chan_rank_empty |= BIT(chan_rank);
+ continue;
+ }
+
+ return -EAGAIN;
+ }
+
+ for (dimm_idx = 0; dimm_idx < dimm_idx_max; dimm_idx++)
+ if (__dimm_temp(pcs, dimm_idx))
+ dimm_mask |= BIT(chan_rank * dimm_idx_max +
dimm_idx);
+ }
+
+ /* If we got all -EINVALs, it means that the CPU doesn't have any
DIMMs. */
+ if (chan_rank_empty == GENMASK(chan_rank_max - 1, 0))
+ return -ENODEV;
+
+ /*
+ * It's possible that memory training is not done yet. In this case
we
+ * defer the detection to be performed at a later point in time.
+ */
+ if (!dimm_mask)
+ return -EAGAIN;
+
+ dev_dbg(priv->dev, "Scanned populated DIMMs: %#llx\n", dimm_mask);
+
+ bitmap_from_u64(priv->dimm_mask, dimm_mask);
+
+ return 0;
+}
+
+static int create_dimm_temp_label(struct peci_dimmtemp *priv, int chan)
+{
+ int rank = chan / priv->gen_info->dimm_idx_max;
+ int idx = chan % priv->gen_info->dimm_idx_max;
+
+ priv->dimmtemp_label[chan] = devm_kasprintf(priv->dev, GFP_KERNEL,
+ "DIMM %c%d", 'A' + rank,
+ idx + 1);
+ if (!priv->dimmtemp_label[chan])
+ return -ENOMEM;
+
+ return 0;
+}
+
+static const u32 peci_dimmtemp_temp_channel_config[] = {
+ [0 ... DIMM_NUMS_MAX - 1] = HWMON_T_LABEL | HWMON_T_INPUT |
HWMON_T_MAX | HWMON_T_CRIT,
+ 0
+};
+
+static const struct hwmon_channel_info peci_dimmtemp_temp_channel = {
+ .type = hwmon_temp,
+ .config = peci_dimmtemp_temp_channel_config,
+};
+
+static const struct hwmon_channel_info *peci_dimmtemp_temp_info[] = {
+ &peci_dimmtemp_temp_channel,
+ NULL
+};
+
+static const struct hwmon_chip_info peci_dimmtemp_chip_info = {
+ .ops = &peci_dimmtemp_ops,
+ .info = peci_dimmtemp_temp_info,
+};
+
+static int create_dimm_temp_info(struct peci_dimmtemp *priv)
+{
+ int ret, i, channels;
+ struct device *dev;
+
+ /*
+ * We expect to either find populated DIMMs and carry on with
creating
+ * sensors, or find out that there are no DIMMs populated.
+ * All other states mean that the platform never reached the state
that
+ * allows to check DIMM state - causing us to retry later on.
+ */
+ ret = check_populated_dimms(priv);
+ if (ret == -ENODEV) {
+ dev_dbg(priv->dev, "No DIMMs found\n");
+ return 0;
+ } else if (ret) {
+ schedule_delayed_work(&priv->detect_work,
DIMM_MASK_CHECK_DELAY_JIFFIES);
+ dev_dbg(priv->dev, "Deferred populating DIMM temp info\n");
+ return ret;
+ }
+
+ channels = priv->gen_info->chan_rank_max * priv->gen_info-
quoted
dimm_idx_max;
+
+ priv->dimmtemp_label = devm_kzalloc(priv->dev, channels *
sizeof(char *), GFP_KERNEL);
+ if (!priv->dimmtemp_label)
+ return -ENOMEM;
+
+ for_each_set_bit(i, priv->dimm_mask, DIMM_NUMS_MAX) {
+ ret = create_dimm_temp_label(priv, i);
+ if (ret)
+ return ret;
+ mutex_init(&priv->dimm[i].thresholds.state.lock);
+ mutex_init(&priv->dimm[i].temp.state.lock);
+ }
+
+ dev = devm_hwmon_device_register_with_info(priv->dev, priv->name,
priv,
+ &peci_dimmtemp_chip_info,
NULL);
+ if (IS_ERR(dev)) {
+ dev_err(priv->dev, "Failed to register hwmon device\n");
+ return PTR_ERR(dev);
+ }
+
+ dev_dbg(priv->dev, "%s: sensor '%s'\n", dev_name(dev), priv->name);
+
+ return 0;
+}
+
+static void create_dimm_temp_info_delayed(struct work_struct *work)
+{
+ struct peci_dimmtemp *priv = container_of(to_delayed_work(work),
+ struct peci_dimmtemp,
+ detect_work);
+ int ret;
+
+ ret = create_dimm_temp_info(priv);
+ if (ret && ret != -EAGAIN)
+ dev_err(priv->dev, "Failed to populate DIMM temp info\n");
+}
+
+static void remove_delayed_work(void *_priv)
+{
+ struct peci_dimmtemp *priv = _priv;
+
+ cancel_delayed_work_sync(&priv->detect_work);
+}
+
+static int peci_dimmtemp_probe(struct auxiliary_device *adev, const struct
auxiliary_device_id *id)
+{
+ struct device *dev = &adev->dev;
+ struct peci_device *peci_dev = to_peci_device(dev->parent);
+ struct peci_dimmtemp *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->name = devm_kasprintf(dev, GFP_KERNEL, "peci_dimmtemp.cpu%d",
+ peci_dev->info.socket_id);
+ if (!priv->name)
+ return -ENOMEM;
+
+ priv->dev = dev;
+ priv->peci_dev = peci_dev;
+ priv->gen_info = (const struct dimm_info *)id->driver_data;
+
+ /*
+ * This is just a sanity check. Since we're using commands that are
+ * guaranteed to be supported on a given platform, we should never
see
+ * revision lower than expected.
+ */
+ if (peci_dev->info.peci_revision < priv->gen_info-
On Tue, 2021-08-03 at 08:39 -0700, Guenter Roeck wrote:
quoted
On Tue, Aug 03, 2021 at 01:31:32PM +0200, Iwona Winiarska wrote:
quoted
Add peci-dimmtemp driver for Temperature Sensor on DIMM readings that
are accessible via the processor PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain thermal readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
Note that the timeout was completely removed - we're going to probe
for detected DIMMs every 5 seconds until we reach "stable" state of
either getting correct DIMM data or getting all -EINVAL (which
suggest that the CPU doesn't have any DIMMs).
drivers/hwmon/peci/Kconfig | 13 +
drivers/hwmon/peci/Makefile | 2 +
drivers/hwmon/peci/dimmtemp.c | 614 ++++++++++++++++++++++++++++++++++
3 files changed, 629 insertions(+)
create mode 100644 drivers/hwmon/peci/dimmtemp.c
hwmon
+ driver which provides Temperature Sensor on DIMM readings that are
+ accessible via the processor PECI interface.
+
+ This driver can also be built as a module. If so, the module
+ will be called peci-dimmtemp.
+
config SENSORS_PECI
tristate
I don't immediately see the value of those build bugs. What happens if
CHAN_RANK_MAX > 32 or DIMM_NUMS_MAX > 64 ? Where do those limits come
from ?
Supported HW doesn't come near the limit for now - it's just an "artificial"
limit imposed by variables we're using (u64 for dimm_mask and u32 for
chan_rank_empty).
Please use a value derived from the size of those variables for the check
to clarify and explain the constraints.
Thanks,
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 2021-08-04 at 10:33 -0700, Guenter Roeck wrote:
On 8/4/21 3:46 AM, Winiarska, Iwona wrote:
quoted
On Tue, 2021-08-03 at 08:39 -0700, Guenter Roeck wrote:
quoted
On Tue, Aug 03, 2021 at 01:31:32PM +0200, Iwona Winiarska wrote:
quoted
Add peci-dimmtemp driver for Temperature Sensor on DIMM readings that
are accessible via the processor PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain thermal readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
Note that the timeout was completely removed - we're going to probe
for detected DIMMs every 5 seconds until we reach "stable" state of
either getting correct DIMM data or getting all -EINVAL (which
suggest that the CPU doesn't have any DIMMs).
drivers/hwmon/peci/Kconfig | 13 +
drivers/hwmon/peci/Makefile | 2 +
drivers/hwmon/peci/dimmtemp.c | 614 ++++++++++++++++++++++++++++++++++
3 files changed, 629 insertions(+)
create mode 100644 drivers/hwmon/peci/dimmtemp.c
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 client"
+ depends on PECI
+ select SENSORS_PECI
+ select PECI_CPU
+ help
+ If you say yes here you get support for the generic Intel PECI
hwmon
+ driver which provides Temperature Sensor on DIMM readings that
are
+ accessible via the processor PECI interface.
+
+ This driver can also be built as a module. If so, the module
+ will be called peci-dimmtemp.
+
config SENSORS_PECI
tristate
I don't immediately see the value of those build bugs. What happens if
CHAN_RANK_MAX > 32 or DIMM_NUMS_MAX > 64 ? Where do those limits come
from ?
Supported HW doesn't come near the limit for now - it's just an "artificial"
limit imposed by variables we're using (u64 for dimm_mask and u32 for
chan_rank_empty).
Please use a value derived from the size of those variables for the check
to clarify and explain the constraints.
Add peci-cputemp driver for Digital Thermal Sensor (DTS) thermal
readings of the processor package and processor cores that are
accessible via the PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain the DTS readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 7 +
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/peci/Kconfig | 18 ++
drivers/hwmon/peci/Makefile | 5 +
drivers/hwmon/peci/common.h | 58 ++++
drivers/hwmon/peci/cputemp.c | 591 +++++++++++++++++++++++++++++++++++
7 files changed, 682 insertions(+)
create mode 100644 drivers/hwmon/peci/Kconfig
create mode 100644 drivers/hwmon/peci/Makefile
create mode 100644 drivers/hwmon/peci/common.h
create mode 100644 drivers/hwmon/peci/cputemp.c
@@ -0,0 +1,591 @@+// SPDX-License-Identifier: GPL-2.0-only+// Copyright (c) 2018-2021 Intel Corporation++#include<linux/auxiliary_bus.h>+#include<linux/bitfield.h>+#include<linux/bitops.h>+#include<linux/hwmon.h>+#include<linux/jiffies.h>+#include<linux/module.h>+#include<linux/peci.h>+#include<linux/peci-cpu.h>+#include<linux/units.h>+#include<linux/x86/intel-family.h>++#include"common.h"++#define CORE_NUMS_MAX 64++#define BASE_CHANNEL_NUMS 5+#define CPUTEMP_CHANNEL_NUMS (BASE_CHANNEL_NUMS + CORE_NUMS_MAX)++#define TEMP_TARGET_FAN_TEMP_MASK GENMASK(15, 8)+#define TEMP_TARGET_REF_TEMP_MASK GENMASK(23, 16)+#define TEMP_TARGET_TJ_OFFSET_MASK GENMASK(29, 24)++#define DTS_MARGIN_MASK GENMASK(15, 0)+#define PCS_MODULE_TEMP_MASK GENMASK(15, 0)++#define DTS_FIXED_POINT_FRACTION 64++structresolved_cores_reg{+u8bus;+u8dev;+u8func;+u8offset;+};++structcpu_info{+structresolved_cores_reg*reg;+u8min_peci_revision;+};++structpeci_temp_target{+s32tcontrol;+s32tthrottle;+s32tjmax;+structpeci_sensor_statestate;+};++enumpeci_temp_target_type{+tcontrol_type,+tthrottle_type,+tjmax_type,+crit_hyst_type,+};++structpeci_cputemp{+structpeci_device*peci_dev;+structdevice*dev;+constchar*name;+conststructcpu_info*gen_info;+struct{+structpeci_temp_targettarget;+structpeci_sensor_datadie;+structpeci_sensor_datadts;+structpeci_sensor_datacore[CORE_NUMS_MAX];+}temp;+constchar**coretemp_label;+DECLARE_BITMAP(core_mask,CORE_NUMS_MAX);+};++enumcputemp_channels{+channel_die,+channel_dts,+channel_tcontrol,+channel_tthrottle,+channel_tjmax,+channel_core,+};++staticconstchar*constcputemp_label[BASE_CHANNEL_NUMS]={+"Die",+"DTS",+"Tcontrol",+"Tthrottle",+"Tjmax",+};++staticintupdate_temp_target(structpeci_cputemp*priv)+{+s32tthrottle_offset,tcontrol_margin;+u32pcs;+intret;++if(!peci_sensor_need_update(&priv->temp.target.state))+return0;++ret=peci_pcs_read(priv->peci_dev,PECI_PCS_TEMP_TARGET,0,&pcs);+if(ret)+returnret;++priv->temp.target.tjmax=+FIELD_GET(TEMP_TARGET_REF_TEMP_MASK,pcs)*MILLIDEGREE_PER_DEGREE;++tcontrol_margin=FIELD_GET(TEMP_TARGET_FAN_TEMP_MASK,pcs);+tcontrol_margin=sign_extend32(tcontrol_margin,7)*MILLIDEGREE_PER_DEGREE;+priv->temp.target.tcontrol=priv->temp.target.tjmax-tcontrol_margin;++tthrottle_offset=FIELD_GET(TEMP_TARGET_TJ_OFFSET_MASK,pcs)*MILLIDEGREE_PER_DEGREE;+priv->temp.target.tthrottle=priv->temp.target.tjmax-tthrottle_offset;++peci_sensor_mark_updated(&priv->temp.target.state);++return0;+}++staticintget_temp_target(structpeci_cputemp*priv,enumpeci_temp_target_typetype,long*val)+{+intret;++mutex_lock(&priv->temp.target.state.lock);++ret=update_temp_target(priv);+if(ret)+gotounlock;++switch(type){+casetcontrol_type:+*val=priv->temp.target.tcontrol;+break;+casetthrottle_type:+*val=priv->temp.target.tthrottle;+break;+casetjmax_type:+*val=priv->temp.target.tjmax;+break;+casecrit_hyst_type:+*val=priv->temp.target.tjmax-priv->temp.target.tcontrol;+break;+default:+ret=-EOPNOTSUPP;+break;+}+unlock:+mutex_unlock(&priv->temp.target.state.lock);++returnret;+}++/*+*ProcessorsreturnavalueofDTSreadinginS10.6fixedpointformat+*(16bits:10-bitsignedmagnitude,6-bitfraction).+*Errorcodes:+*0x8000:Generalsensorerror+*0x8001:Reserved+*0x8002:Underflowonreadingvalue+*0x8003-0x81ff:Reserved+*/+staticbooldts_valid(s32val)+{+returnval<0x8000||val>0x81ff;+}++statics32dts_to_millidegree(s32val)+{+returnsign_extend32(val,15)*MILLIDEGREE_PER_DEGREE/DTS_FIXED_POINT_FRACTION;+}++staticintget_die_temp(structpeci_cputemp*priv,long*val)+{+longtjmax;+s16temp;+intret;++mutex_lock(&priv->temp.die.state.lock);+if(!peci_sensor_need_update(&priv->temp.die.state))+gotoskip_update;++ret=peci_temp_read(priv->peci_dev,&temp);+if(ret)+gotoerr_unlock;++if(!dts_valid(temp)){+ret=-EIO;+gotoerr_unlock;+}++ret=get_temp_target(priv,tjmax_type,&tjmax);+if(ret)+gotoerr_unlock;++priv->temp.die.value=(s32)tjmax+dts_to_millidegree(temp);++peci_sensor_mark_updated(&priv->temp.die.state);++skip_update:+*val=priv->temp.die.value;+mutex_unlock(&priv->temp.die.state.lock);++return0;++err_unlock:+mutex_unlock(&priv->temp.die.state.lock);+returnret;+}++staticintget_dts(structpeci_cputemp*priv,long*val)+{+s32dts_margin;+longtcontrol;+u32pcs;+intret;++mutex_lock(&priv->temp.dts.state.lock);+if(!peci_sensor_need_update(&priv->temp.dts.state))+gotoskip_update;++ret=peci_pcs_read(priv->peci_dev,PECI_PCS_THERMAL_MARGIN,0,&pcs);+if(ret)+gotoerr_unlock;++dts_margin=FIELD_GET(DTS_MARGIN_MASK,pcs);+if(!dts_valid(dts_margin)){+ret=-EIO;+gotoerr_unlock;+}++ret=get_temp_target(priv,tcontrol_type,&tcontrol);+if(ret)+gotoerr_unlock;++/* Note that the tcontrol should be available before calling it */+priv->temp.dts.value=(s32)tcontrol-dts_to_millidegree(dts_margin);++peci_sensor_mark_updated(&priv->temp.dts.state);++skip_update:+*val=priv->temp.dts.value;+mutex_unlock(&priv->temp.dts.state.lock);++return0;++err_unlock:+mutex_unlock(&priv->temp.dts.state.lock);+returnret;+}++staticintget_core_temp(structpeci_cputemp*priv,intcore_index,long*val)+{+s32core_dts_margin;+longtjmax;+u32pcs;+intret;++mutex_lock(&priv->temp.core[core_index].state.lock);+if(!peci_sensor_need_update(&priv->temp.core[core_index].state))+gotoskip_update;++ret=peci_pcs_read(priv->peci_dev,PECI_PCS_MODULE_TEMP,core_index,&pcs);+if(ret)+gotoerr_unlock;++core_dts_margin=FIELD_GET(PCS_MODULE_TEMP_MASK,pcs);+if(!dts_valid(core_dts_margin)){+ret=-EIO;+gotoerr_unlock;+}++ret=get_temp_target(priv,tjmax_type,&tjmax);+if(ret)+gotoerr_unlock;++/* Note that the tjmax should be available before calling it */+priv->temp.core[core_index].value=(s32)tjmax+dts_to_millidegree(core_dts_margin);++peci_sensor_mark_updated(&priv->temp.core[core_index].state);++skip_update:+*val=priv->temp.core[core_index].value;+mutex_unlock(&priv->temp.core[core_index].state.lock);++return0;++err_unlock:+mutex_unlock(&priv->temp.core[core_index].state.lock);+returnret;+}++staticintcputemp_read_string(structdevice*dev,enumhwmon_sensor_typestype,+u32attr,intchannel,constchar**str)+{+structpeci_cputemp*priv=dev_get_drvdata(dev);++if(attr!=hwmon_temp_label)+return-EOPNOTSUPP;++*str=channel<channel_core?+cputemp_label[channel]:priv->coretemp_label[channel-channel_core];++return0;+}++staticintcputemp_read(structdevice*dev,enumhwmon_sensor_typestype,+u32attr,intchannel,long*val)+{+structpeci_cputemp*priv=dev_get_drvdata(dev);++switch(attr){+casehwmon_temp_input:+switch(channel){+casechannel_die:+returnget_die_temp(priv,val);+casechannel_dts:+returnget_dts(priv,val);+casechannel_tcontrol:+returnget_temp_target(priv,tcontrol_type,val);+casechannel_tthrottle:+returnget_temp_target(priv,tthrottle_type,val);+casechannel_tjmax:+returnget_temp_target(priv,tjmax_type,val);+default:+returnget_core_temp(priv,channel-channel_core,val);+}+break;+casehwmon_temp_max:+returnget_temp_target(priv,tcontrol_type,val);+casehwmon_temp_crit:+returnget_temp_target(priv,tjmax_type,val);+casehwmon_temp_crit_hyst:+returnget_temp_target(priv,crit_hyst_type,val);+default:+return-EOPNOTSUPP;+}++return0;+}++staticumode_tcputemp_is_visible(constvoid*data,enumhwmon_sensor_typestype,+u32attr,intchannel)+{+conststructpeci_cputemp*priv=data;++if(channel>CPUTEMP_CHANNEL_NUMS)+return0;++if(channel<channel_core)+return0444;++if(test_bit(channel-channel_core,priv->core_mask))+return0444;++return0;+}++staticintinit_core_mask(structpeci_cputemp*priv)+{+structpeci_device*peci_dev=priv->peci_dev;+structresolved_cores_reg*reg=priv->gen_info->reg;+u64core_mask;+u32data;+intret;++/* Get the RESOLVED_CORES register value */+switch(peci_dev->info.model){+caseINTEL_FAM6_ICELAKE_X:+caseINTEL_FAM6_ICELAKE_D:+ret=peci_ep_pci_local_read(peci_dev,0,reg->bus,reg->dev,+reg->func,reg->offset+4,&data);+if(ret)+returnret;++core_mask=(u64)data<<32;++ret=peci_ep_pci_local_read(peci_dev,0,reg->bus,reg->dev,+reg->func,reg->offset,&data);+if(ret)+returnret;++core_mask|=data;++break;+default:+ret=peci_pci_local_read(peci_dev,reg->bus,reg->dev,+reg->func,reg->offset,&data);+if(ret)+returnret;++core_mask=data;++break;+}++if(!core_mask)+return-EIO;++bitmap_from_u64(priv->core_mask,core_mask);++return0;+}++staticintcreate_temp_label(structpeci_cputemp*priv)+{+unsignedlongcore_max=find_last_bit(priv->core_mask,CORE_NUMS_MAX);+inti;++priv->coretemp_label=devm_kzalloc(priv->dev,core_max*sizeof(char*),GFP_KERNEL);+if(!priv->coretemp_label)+return-ENOMEM;++for_each_set_bit(i,priv->core_mask,CORE_NUMS_MAX){+priv->coretemp_label[i]=devm_kasprintf(priv->dev,GFP_KERNEL,"Core %d",i);+if(!priv->coretemp_label[i])+return-ENOMEM;+}++return0;+}++staticvoidcheck_resolved_cores(structpeci_cputemp*priv)+{+/*+*Failuretoresolvecoresisnon-critical,we'restillableto+*provideothersensordata.+*/++if(init_core_mask(priv))+return;++if(create_temp_label(priv))+bitmap_zero(priv->core_mask,CORE_NUMS_MAX);+}++staticvoidsensor_init(structpeci_cputemp*priv)+{+inti;++mutex_init(&priv->temp.target.state.lock);+mutex_init(&priv->temp.die.state.lock);+mutex_init(&priv->temp.dts.state.lock);++for_each_set_bit(i,priv->core_mask,CORE_NUMS_MAX)+mutex_init(&priv->temp.core[i].state.lock);+}++staticconststructhwmon_opspeci_cputemp_ops={+.is_visible=cputemp_is_visible,+.read_string=cputemp_read_string,+.read=cputemp_read,+};++staticconstu32peci_cputemp_temp_channel_config[]={+/* Die temperature */+HWMON_T_LABEL|HWMON_T_INPUT|HWMON_T_MAX|HWMON_T_CRIT|HWMON_T_CRIT_HYST,+/* DTS margin */+HWMON_T_LABEL|HWMON_T_INPUT|HWMON_T_MAX|HWMON_T_CRIT|HWMON_T_CRIT_HYST,+/* Tcontrol temperature */+HWMON_T_LABEL|HWMON_T_INPUT|HWMON_T_CRIT,+/* Tthrottle temperature */+HWMON_T_LABEL|HWMON_T_INPUT,+/* Tjmax temperature */+HWMON_T_LABEL|HWMON_T_INPUT,+/* Core temperature - for all core channels */+[channel_core...CPUTEMP_CHANNEL_NUMS-1]=HWMON_T_LABEL|HWMON_T_INPUT,+0+};++staticconststructhwmon_channel_infopeci_cputemp_temp_channel={+.type=hwmon_temp,+.config=peci_cputemp_temp_channel_config,+};++staticconststructhwmon_channel_info*peci_cputemp_info[]={+&peci_cputemp_temp_channel,+NULL+};++staticconststructhwmon_chip_infopeci_cputemp_chip_info={+.ops=&peci_cputemp_ops,+.info=peci_cputemp_info,+};++staticintpeci_cputemp_probe(structauxiliary_device*adev,+conststructauxiliary_device_id*id)+{+structdevice*dev=&adev->dev;+structpeci_device*peci_dev=to_peci_device(dev->parent);+structpeci_cputemp*priv;+structdevice*hwmon_dev;++priv=devm_kzalloc(dev,sizeof(*priv),GFP_KERNEL);+if(!priv)+return-ENOMEM;++priv->name=devm_kasprintf(dev,GFP_KERNEL,"peci_cputemp.cpu%d",+peci_dev->info.socket_id);+if(!priv->name)+return-ENOMEM;++priv->dev=dev;+priv->peci_dev=peci_dev;+priv->gen_info=(conststructcpu_info*)id->driver_data;++/*+*Thisisjustasanitycheck.Sincewe'reusingcommandsthatare+*guaranteedtobesupportedonagivenplatform,weshouldneversee+*revisionlowerthanexpected.+*/+if(peci_dev->info.peci_revision<priv->gen_info->min_peci_revision)+dev_warn(priv->dev,+"Unexpected PECI revision %#x, some features may be unavailable\n",+peci_dev->info.peci_revision);++check_resolved_cores(priv);++sensor_init(priv);++hwmon_dev=devm_hwmon_device_register_with_info(priv->dev,priv->name,+priv,&peci_cputemp_chip_info,NULL);++returnPTR_ERR_OR_ZERO(hwmon_dev);+}++/*+*RESOLVED_CORESPCIconfigurationregistermayhavedifferentlocationon+*differentplatforms.+*/+staticstructresolved_cores_regresolved_cores_reg_hsx={+.bus=1,+.dev=30,+.func=3,+.offset=0xb4,+};++staticstructresolved_cores_regresolved_cores_reg_icx={+.bus=14,+.dev=30,+.func=3,+.offset=0xd0,+};++staticconststructcpu_infocpu_hsx={+.reg=&resolved_cores_reg_hsx,+.min_peci_revision=0x33,+};++staticconststructcpu_infocpu_icx={+.reg=&resolved_cores_reg_icx,+.min_peci_revision=0x40,+};++staticconststructauxiliary_device_idpeci_cputemp_ids[]={+{+.name="peci_cpu.cputemp.hsx",+.driver_data=(kernel_ulong_t)&cpu_hsx,+},+{+.name="peci_cpu.cputemp.bdx",+.driver_data=(kernel_ulong_t)&cpu_hsx,+},+{+.name="peci_cpu.cputemp.bdxd",+.driver_data=(kernel_ulong_t)&cpu_hsx,+},+{+.name="peci_cpu.cputemp.skx",+.driver_data=(kernel_ulong_t)&cpu_hsx,+},+{+.name="peci_cpu.cputemp.icx",+.driver_data=(kernel_ulong_t)&cpu_icx,+},+{+.name="peci_cpu.cputemp.icxd",+.driver_data=(kernel_ulong_t)&cpu_icx,+},+{}+};+MODULE_DEVICE_TABLE(auxiliary,peci_cputemp_ids);++staticstructauxiliary_driverpeci_cputemp_driver={+.probe=peci_cputemp_probe,+.id_table=peci_cputemp_ids,+};++module_auxiliary_driver(peci_cputemp_driver);++MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");+MODULE_AUTHOR("Iwona Winiarska <iwona.winiarska@intel.com>");+MODULE_DESCRIPTION("PECI cputemp driver");+MODULE_LICENSE("GPL");+MODULE_IMPORT_NS(PECI_CPU);
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Add peci-cputemp driver for Digital Thermal Sensor (DTS) thermal
readings of the processor package and processor cores that are
accessible via the PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain the DTS readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 7 +
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/peci/Kconfig | 18 ++
drivers/hwmon/peci/Makefile | 5 +
drivers/hwmon/peci/common.h | 58 ++++
drivers/hwmon/peci/cputemp.c | 591 +++++++++++++++++++++++++++++++++++
7 files changed, 682 insertions(+)
create mode 100644 drivers/hwmon/peci/Kconfig
create mode 100644 drivers/hwmon/peci/Makefile
create mode 100644 drivers/hwmon/peci/common.h
create mode 100644 drivers/hwmon/peci/cputemp.c
On Tue, 2021-08-03 at 08:24 -0700, Guenter Roeck wrote:
On 8/3/21 4:31 AM, Iwona Winiarska wrote:
quoted
Add peci-cputemp driver for Digital Thermal Sensor (DTS) thermal
readings of the processor package and processor cores that are
accessible via the PECI interface.
The main use case for the driver (and PECI interface) is out-of-band
management, where we're able to obtain the DTS readings from an external
entity connected with PECI, e.g. BMC on server platforms.
Co-developed-by: Jae Hyun Yoo <redacted>
Signed-off-by: Jae Hyun Yoo <redacted>
Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
Reviewed-by: Pierre-Louis Bossart <redacted>
---
MAINTAINERS | 7 +
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/peci/Kconfig | 18 ++
drivers/hwmon/peci/Makefile | 5 +
drivers/hwmon/peci/common.h | 58 ++++
drivers/hwmon/peci/cputemp.c | 591 +++++++++++++++++++++++++++++++++++
7 files changed, 682 insertions(+)
create mode 100644 drivers/hwmon/peci/Kconfig
create mode 100644 drivers/hwmon/peci/Makefile
create mode 100644 drivers/hwmon/peci/common.h
create mode 100644 drivers/hwmon/peci/cputemp.c
These devices are hard to detect and rarely found on mainstream
hardware. If unsure, say N.
+source "drivers/hwmon/peci/Kconfig"
+
source "drivers/hwmon/pmbus/Kconfig"
config SENSORS_PWM_FAN
@@ -0,0 +1,90 @@+.. SPDX-License-Identifier: GPL-2.0-only++Kernel driver peci-cputemp+==========================++Supported chips:+ One of Intel server CPUs listed below which is connected to a PECI bus.+* Intel Xeon E5/E7 v3 server processors+ Intel Xeon E5-14xx v3 family+ Intel Xeon E5-24xx v3 family+ Intel Xeon E5-16xx v3 family+ Intel Xeon E5-26xx v3 family+ Intel Xeon E5-46xx v3 family+ Intel Xeon E7-48xx v3 family+ Intel Xeon E7-88xx v3 family+* Intel Xeon E5/E7 v4 server processors+ Intel Xeon E5-16xx v4 family+ Intel Xeon E5-26xx v4 family+ Intel Xeon E5-46xx v4 family+ Intel Xeon E7-48xx v4 family+ Intel Xeon E7-88xx v4 family+* Intel Xeon Scalable server processors+ Intel Xeon D family+ Intel Xeon Bronze family+ Intel Xeon Silver family+ Intel Xeon Gold family+ Intel Xeon Platinum family++ Datasheet: Available from http://www.intel.com/design/literature.htm++Author: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>++Description+-----------++This driver implements a generic PECI hwmon feature which provides Digital+Thermal Sensor (DTS) thermal readings of the CPU package and CPU cores that are+accessible via the processor PECI interface.++All temperature values are given in millidegree Celsius and will be measurable+only when the target CPU is powered on.++Sysfs interface+-------------------++======================= =======================================================+temp1_label "Die"+temp1_input Provides current die temperature of the CPU package.+temp1_max Provides thermal control temperature of the CPU package+ which is also known as Tcontrol.+temp1_crit Provides shutdown temperature of the CPU package which+ is also known as the maximum processor junction+ temperature, Tjmax or Tprochot.+temp1_crit_hyst Provides the hysteresis value from Tcontrol to Tjmax of+ the CPU package.++temp2_label "DTS"+temp2_input Provides current temperature of the CPU package scaled+ to match DTS thermal profile.+temp2_max Provides thermal control temperature of the CPU package+ which is also known as Tcontrol.+temp2_crit Provides shutdown temperature of the CPU package which+ is also known as the maximum processor junction+ temperature, Tjmax or Tprochot.+temp2_crit_hyst Provides the hysteresis value from Tcontrol to Tjmax of+ the CPU package.++temp3_label "Tcontrol"+temp3_input Provides current Tcontrol temperature of the CPU+ package which is also known as Fan Temperature target.+ Indicates the relative value from thermal monitor trip+ temperature at which fans should be engaged.+temp3_crit Provides Tcontrol critical value of the CPU package+ which is same to Tjmax.++temp4_label "Tthrottle"+temp4_input Provides current Tthrottle temperature of the CPU+ package. Used for throttling temperature. If this value+ is allowed and lower than Tjmax - the throttle will+ occur and reported at lower than Tjmax.++temp5_label "Tjmax"+temp5_input Provides the maximum junction temperature, Tjmax of the+ CPU package.++temp[6-N]_label Provides string "Core X", where X is resolved core+ number.+temp[6-N]_input Provides current temperature of each core.++======================= =======================================================
@@ -0,0 +1,57 @@+.. SPDX-License-Identifier: GPL-2.0++Kernel driver peci-dimmtemp+===========================++Supported chips:+ One of Intel server CPUs listed below which is connected to a PECI bus.+* Intel Xeon E5/E7 v3 server processors+ Intel Xeon E5-14xx v3 family+ Intel Xeon E5-24xx v3 family+ Intel Xeon E5-16xx v3 family+ Intel Xeon E5-26xx v3 family+ Intel Xeon E5-46xx v3 family+ Intel Xeon E7-48xx v3 family+ Intel Xeon E7-88xx v3 family+* Intel Xeon E5/E7 v4 server processors+ Intel Xeon E5-16xx v4 family+ Intel Xeon E5-26xx v4 family+ Intel Xeon E5-46xx v4 family+ Intel Xeon E7-48xx v4 family+ Intel Xeon E7-88xx v4 family+* Intel Xeon Scalable server processors+ Intel Xeon D family+ Intel Xeon Bronze family+ Intel Xeon Silver family+ Intel Xeon Gold family+ Intel Xeon Platinum family++ Datasheet: Available from http://www.intel.com/design/literature.htm++Author: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>++Description+-----------++This driver implements a generic PECI hwmon feature which provides+Temperature sensor on DIMM readings that are accessible via the processor PECI interface.++All temperature values are given in millidegree Celsius and will be measurable+only when the target CPU is powered on.++Sysfs interface+-------------------++======================= =======================================================++temp[N]_label Provides string "DIMM CI", where C is DIMM channel and+ I is DIMM index of the populated DIMM.+temp[N]_input Provides current temperature of the populated DIMM.+temp[N]_max Provides thermal control temperature of the DIMM.+temp[N]_crit Provides shutdown temperature of the DIMM.++======================= =======================================================++Note:+ DIMM temperature attributes will appear when the client CPU's BIOS+ completes memory training and testing.
@@ -0,0 +1,48 @@+.. SPDX-License-Identifier: GPL-2.0-only++========+Overview+========++The Platform Environment Control Interface (PECI) is a communication+interface between Intel processor and management controllers+(e.g. Baseboard Management Controller, BMC).+PECI provides services that allow the management controller to+configure, monitor and debug platform by accessing various registers.+It defines a dedicated command protocol, where the management+controller is acting as a PECI originator and the processor - as+a PECI responder.+PECI can be used in both single processor and multiple-processor based+systems.++NOTE:+Intel PECI specification is not released as a dedicated document,+instead it is a part of External Design Specification (EDS) for given+Intel CPU. External Design Specifications are usually not publicly+available.++PECI Wire+---------++PECI Wire interface uses a single wire for self-clocking and data+transfer. It does not require any additional control lines - the+physical layer is a self-clocked one-wire bus signal that begins each+bit with a driven, rising edge from an idle near zero volts. The+duration of the signal driven high allows to determine whether the bit+value is logic '0' or logic '1'. PECI Wire also includes variable data+rate established with every message.++For PECI Wire, each processor package will utilize unique, fixed+addresses within a defined range and that address should+have a fixed relationship with the processor socket ID - if one of the+processors is removed, it does not affect addresses of remaining+processors.++PECI subsystem internals+------------------------++..kernel-doc:: include/linux/peci.h++PECI CPU Driver API+-------------------+..kernel-doc:: include/linux/peci-cpu.h
On Tue, Aug 03, 2021 at 01:31:19PM +0200, Iwona Winiarska wrote:
Hi Greg,
This is a second round of patches introducing PECI subsystem.
I don't think it is ready to be applied right away (we're still
missing r-b's), but I hope we have chance to complete discussion in
the 5.15 development cycle. I would appreciate if you could take
a look.
I will wait to review this when you all feel it is ready so as to not
waste my time finding things that you already know need to be resolved.
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel