Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

Hi!

> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>  .../devicetree/bindings/peci/peci-aspeed.txt       | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
> 
> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> new file mode 100644
> index 000000000000..8a86f346d550
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> @@ -0,0 +1,73 @@
> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.

Are these SoCs x86-based?

> +Required properties:
> +- compatible
> +	"aspeed,ast2400-peci" or "aspeed,ast2500-peci"
> +	- aspeed,ast2400-peci: Aspeed AST2400 family PECI controller
> +	- aspeed,ast2500-peci: Aspeed AST2500 family PECI controller
> +
> +- reg
> +	Should contain PECI registers location and length.

Other dts documents put it on one line, reg: Should contain ...

> +- clock_frequency
> +	Should contain the operation frequency of PECI hardware module.
> +	187500 ~ 24000000

specify this is Hz?

> +- rd-sampling-point
> +	Read sampling point selection. The whole period of a bit time will be
> +	divided into 16 time frames. This value will determine which time frame
> +	this controller will sample PECI signal for data read back. Usually in
> +	the middle of a bit time is the best.

English? "This value will determine when this controller"?

> +	0 ~ 15 (default: 8)
> +
> +- cmd_timeout_ms
> +	Command timeout in units of ms.
> +	1 ~ 60000 (default: 1000)
> +
> +Example:
> +	peci: peci@1e78b000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x1e78b000 0x60>;
> +
> +		peci0: peci-bus@0 {
> +			compatible = "aspeed,ast2500-peci";
> +			reg = <0x0 0x60>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <15>;
> +			clocks = <&clk_clkin>;
> +			clock-frequency = <24000000>;
> +			msg-timing-nego = <1>;
> +			addr-timing-nego = <1>;
> +			rd-sampling-point = <8>;
> +			cmd-timeout-ms = <1000>;
> +		};
> +	};
> \ No newline at end of file

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 0/8] PECI device driver introduction

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]

Hi!

> Introduction of the Platform Environment Control Interface (PECI) bus
> device driver. PECI is a one-wire bus interface that provides a
> communication channel between Intel processor and chipset components to
> external monitoring or control devices. PECI is designed to support the
> following sideband functions:
> 
> * Processor and DRAM thermal management
>   - Processor fan speed control is managed by comparing Digital Thermal
>     Sensor (DTS) thermal readings acquired via PECI against the
>     processor-specific fan speed control reference point, or TCONTROL.
>     Both TCONTROL and DTS thermal readings are accessible via the processor
>     PECI client. These variables are referenced to a common temperature,
>     the TCC activation point, and are both defined as negative offsets from
>     that reference.
>   - PECI based access to the processor package configuration space provides
>     a means for Baseboard Management Controllers (BMC) or other platform
>     management devices to actively manage the processor and memory power
>     and thermal features.
> 
> * Platform Manageability
>   - Platform manageability functions including thermal, power, and error
>     monitoring. Note that platform 'power' management includes monitoring
>     and control for both the processor and DRAM subsystem to assist with
>     data center power limiting.
>   - PECI allows read access to certain error registers in the processor MSR
>     space and status monitoring registers in the PCI configuration space
>     within the processor and downstream devices.
>   - PECI permits writes to certain registers in the processor PCI
>     configuration space.
> 
> * Processor Interface Tuning and Diagnostics
>   - Processor interface tuning and diagnostics capabilities
>     (Intel(c) Interconnect BIST). The processors Intel(c) Interconnect
>     Built In Self Test (Intel(c) IBIST) allows for infield diagnostic
>     capabilities in the Intel UPI and memory controller interfaces. PECI
>     provides a port to execute these diagnostics via its PCI Configuration
>     read and write capabilities.
> 
> * Failure Analysis
>   - Output the state of the processor after a failure for analysis via
>     Crashdump.
> 
> PECI uses a single wire for self-clocking and data transfer. The bus
> requires no additional control lines. The physical layer is a self-clocked
> one-wire bus that begins each bit with a driven, rising edge from an idle
> level near zero volts. The duration of the signal driven high depends on
> whether the bit value is a logic '0' or logic '1'. PECI also includes
> variable data transfer rate established with every message. In this way,
> it is highly flexible even though underlying logic is simple.
> 
> The interface design was optimized for interfacing to Intel processor and
> chipset components in both single processor and multiple processor
> environments. The single wire interface provides low board routing
> overhead for the multiple load connections in the congested routing area
> near the processor and chipset components. Bus speed, error checking, and
> low protocol overhead provides adequate link bandwidth and reliability to
> transfer critical device operating conditions and configuration
> information.
> 
> This implementation provides the basic framework to add PECI extensions
> to the Linux bus and device models. A hardware specific 'Adapter' driver
> can be attached to the PECI bus to provide sideband functions described
> above. It is also possible to access all devices on an adapter from
> userspace through the /dev interface. A device specific 'Client' driver
> also can be attached to the PECI bus so each processor client's features
> can be supported by the 'Client' driver through an adapter connection in
> the bus. This patch set includes Aspeed 24xx/25xx PECI driver and a generic
> PECI hwmon driver as the first implementation for both adapter and client
> drivers on the PECI bus framework.

Ok, how does this interact with ACPI/SMM BIOS/Secure mode code? Does
Linux _need_ to control the fan? Or is SMM BIOS capable of doing all
the work itself and Linux has just read-only access for monitoring
purposes?

Pavel

-- (english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Andrew Lunn]

On Tue, Mar 06, 2018 at 01:40:02PM +0100, Pavel Machek wrote:
> Hi!
> 
> > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > ---
> >  .../devicetree/bindings/peci/peci-aspeed.txt       | 73 ++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > new file mode 100644
> > index 000000000000..8a86f346d550
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > @@ -0,0 +1,73 @@
> > +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
> 
> Are these SoCs x86-based?

ARM, as far as i can tell. If i get the architecture correct, these
are BMC, Board Management Controllers, looking after the main x86 CPU,
stopping it overheating, controlling the power supplies, remote
management, etc.

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

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Tue 2018-03-06 13:54:16, Andrew Lunn wrote:
> On Tue, Mar 06, 2018 at 01:40:02PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > > ---
> > >  .../devicetree/bindings/peci/peci-aspeed.txt       | 73 ++++++++++++++++++++++
> > >  1 file changed, 73 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > > new file mode 100644
> > > index 000000000000..8a86f346d550
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > > @@ -0,0 +1,73 @@
> > > +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
> > 
> > Are these SoCs x86-based?
> 
> ARM, as far as i can tell. If i get the architecture correct, these
> are BMC, Board Management Controllers, looking after the main x86 CPU,
> stopping it overheating, controlling the power supplies, remote
> management, etc.

Ok, so with x86 machine, I get arm-based one for free. I get it. Is
user able to run his own kernel on the arm system, or is it locked
down, TiVo style?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Arnd Bergmann]

On Tue, Mar 6, 2018 at 2:05 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Tue 2018-03-06 13:54:16, Andrew Lunn wrote:
>> On Tue, Mar 06, 2018 at 01:40:02PM +0100, Pavel Machek wrote:
>> > Hi!
>> >
>> > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> > > ---
>> > >  .../devicetree/bindings/peci/peci-aspeed.txt       | 73 ++++++++++++++++++++++
>> > >  1 file changed, 73 insertions(+)
>> > >  create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> > >
>> > > diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> > > new file mode 100644
>> > > index 000000000000..8a86f346d550
>> > > --- /dev/null
>> > > +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> > > @@ -0,0 +1,73 @@
>> > > +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
>> >
>> > Are these SoCs x86-based?
>>
>> ARM, as far as i can tell. If i get the architecture correct, these
>> are BMC, Board Management Controllers, looking after the main x86 CPU,
>> stopping it overheating, controlling the power supplies, remote
>> management, etc.
>
> Ok, so with x86 machine, I get arm-based one for free. I get it. Is
> user able to run his own kernel on the arm system, or is it locked
> down, TiVo style?

In the past, they were all locked down, the team submitting those
patches in working on changing that. Have a look for OpenBMC.

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

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Jae Hyun Yoo]

Hi Pavel,

Thanks for sharing your time on reviewing it. Please see my answers inline.

-Jae

On 3/6/2018 4:40 AM, Pavel Machek wrote:
> Hi!
> 
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>>   .../devicetree/bindings/peci/peci-aspeed.txt       | 73 ++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>>
>> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> new file mode 100644
>> index 000000000000..8a86f346d550
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> @@ -0,0 +1,73 @@
>> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
> 
> Are these SoCs x86-based?
> 

Yes, these are ARM SoCs. Please see Andrew's answer as well.

>> +Required properties:
>> +- compatible
>> +	"aspeed,ast2400-peci" or "aspeed,ast2500-peci"
>> +	- aspeed,ast2400-peci: Aspeed AST2400 family PECI controller
>> +	- aspeed,ast2500-peci: Aspeed AST2500 family PECI controller
>> +
>> +- reg
>> +	Should contain PECI registers location and length.
> 
> Other dts documents put it on one line, reg: Should contain ...
> 
>> +- clock_frequency
>> +	Should contain the operation frequency of PECI hardware module.
>> +	187500 ~ 24000000
> 
> specify this is Hz?
> 

I'll add a description. Thanks!

>> +- rd-sampling-point
>> +	Read sampling point selection. The whole period of a bit time will be
>> +	divided into 16 time frames. This value will determine which time frame
>> +	this controller will sample PECI signal for data read back. Usually in
>> +	the middle of a bit time is the best.
> 
> English? "This value will determine when this controller"?
> 

Could I change it like below?:

"This value will determine in which time frame this controller samples 
PECI signal for data read back"

>> +	0 ~ 15 (default: 8)
>> +
>> +- cmd_timeout_ms
>> +	Command timeout in units of ms.
>> +	1 ~ 60000 (default: 1000)
>> +
>> +Example:
>> +	peci: peci@1e78b000 {
>> +		compatible = "simple-bus";
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x1e78b000 0x60>;
>> +
>> +		peci0: peci-bus@0 {
>> +			compatible = "aspeed,ast2500-peci";
>> +			reg = <0x0 0x60>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			interrupts = <15>;
>> +			clocks = <&clk_clkin>;
>> +			clock-frequency = <24000000>;
>> +			msg-timing-nego = <1>;
>> +			addr-timing-nego = <1>;
>> +			rd-sampling-point = <8>;
>> +			cmd-timeout-ms = <1000>;
>> +		};
>> +	};
>> \ No newline at end of file
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/8] PECI device driver introduction

[Jae Hyun Yoo]

Hi Pavel,

Please see my answer inline.

On 3/6/2018 4:40 AM, Pavel Machek wrote:
> Hi!
> 
>> Introduction of the Platform Environment Control Interface (PECI) bus
>> device driver. PECI is a one-wire bus interface that provides a
>> communication channel between Intel processor and chipset components to
>> external monitoring or control devices. PECI is designed to support the
>> following sideband functions:
>>
>> * Processor and DRAM thermal management
>>    - Processor fan speed control is managed by comparing Digital Thermal
>>      Sensor (DTS) thermal readings acquired via PECI against the
>>      processor-specific fan speed control reference point, or TCONTROL.
>>      Both TCONTROL and DTS thermal readings are accessible via the processor
>>      PECI client. These variables are referenced to a common temperature,
>>      the TCC activation point, and are both defined as negative offsets from
>>      that reference.
>>    - PECI based access to the processor package configuration space provides
>>      a means for Baseboard Management Controllers (BMC) or other platform
>>      management devices to actively manage the processor and memory power
>>      and thermal features.
>>
>> * Platform Manageability
>>    - Platform manageability functions including thermal, power, and error
>>      monitoring. Note that platform 'power' management includes monitoring
>>      and control for both the processor and DRAM subsystem to assist with
>>      data center power limiting.
>>    - PECI allows read access to certain error registers in the processor MSR
>>      space and status monitoring registers in the PCI configuration space
>>      within the processor and downstream devices.
>>    - PECI permits writes to certain registers in the processor PCI
>>      configuration space.
>>
>> * Processor Interface Tuning and Diagnostics
>>    - Processor interface tuning and diagnostics capabilities
>>      (Intel(c) Interconnect BIST). The processors Intel(c) Interconnect
>>      Built In Self Test (Intel(c) IBIST) allows for infield diagnostic
>>      capabilities in the Intel UPI and memory controller interfaces. PECI
>>      provides a port to execute these diagnostics via its PCI Configuration
>>      read and write capabilities.
>>
>> * Failure Analysis
>>    - Output the state of the processor after a failure for analysis via
>>      Crashdump.
>>
>> PECI uses a single wire for self-clocking and data transfer. The bus
>> requires no additional control lines. The physical layer is a self-clocked
>> one-wire bus that begins each bit with a driven, rising edge from an idle
>> level near zero volts. The duration of the signal driven high depends on
>> whether the bit value is a logic '0' or logic '1'. PECI also includes
>> variable data transfer rate established with every message. In this way,
>> it is highly flexible even though underlying logic is simple.
>>
>> The interface design was optimized for interfacing to Intel processor and
>> chipset components in both single processor and multiple processor
>> environments. The single wire interface provides low board routing
>> overhead for the multiple load connections in the congested routing area
>> near the processor and chipset components. Bus speed, error checking, and
>> low protocol overhead provides adequate link bandwidth and reliability to
>> transfer critical device operating conditions and configuration
>> information.
>>
>> This implementation provides the basic framework to add PECI extensions
>> to the Linux bus and device models. A hardware specific 'Adapter' driver
>> can be attached to the PECI bus to provide sideband functions described
>> above. It is also possible to access all devices on an adapter from
>> userspace through the /dev interface. A device specific 'Client' driver
>> also can be attached to the PECI bus so each processor client's features
>> can be supported by the 'Client' driver through an adapter connection in
>> the bus. This patch set includes Aspeed 24xx/25xx PECI driver and a generic
>> PECI hwmon driver as the first implementation for both adapter and client
>> drivers on the PECI bus framework.
> 
> Ok, how does this interact with ACPI/SMM BIOS/Secure mode code? Does
> Linux _need_ to control the fan? Or is SMM BIOS capable of doing all
> the work itself and Linux has just read-only access for monitoring
> purposes?
> 

This driver is not for local CPUs which this driver is running on. 
Instead, this driver will be running on BMC (Baseboard Management 
Controller) kernel which is separated from the server machine. In this 
implementation, it provides just read-only access for monitoring the 
server's CPU and DIMM temperatures remotely through a PECI connection. 
The BMC can control fans according to the monitoring data if the BMC has 
a fan control interface and feature, but it depends on baseboard 
hardware and software designs.

Thanks,
Jae

> Pavel
> 
> -- (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/8] [PATCH 6/8] Documentation: hwmon: Add a document for PECI hwmon client driver

[Randy Dunlap]

Hi,

On 02/21/2018 08:16 AM, Jae Hyun Yoo wrote:

> +temp<n>_label		Provides DDR DIMM temperature if this label indicates
> +			'DIMM #'.
> +temp<n>_input		Provides current temperature of the DDR DIMM.
> +
> +Note:
> +	DIMM temperature group will be appeared when the client CPU's BIOS

	                       will appear when

> +	completes memory training and testing.
> 


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

Re: [PATCH v2 6/8] [PATCH 6/8] Documentation: hwmon: Add a document for PECI hwmon client driver

[Jae Hyun Yoo]

Hi Randy,

On 3/6/2018 12:28 PM, Randy Dunlap wrote:
> Hi,
> 
> On 02/21/2018 08:16 AM, Jae Hyun Yoo wrote:
> 
>> +temp<n>_label		Provides DDR DIMM temperature if this label indicates
>> +			'DIMM #'.
>> +temp<n>_input		Provides current temperature of the DDR DIMM.
>> +
>> +Note:
>> +	DIMM temperature group will be appeared when the client CPU's BIOS
> 
> 	                       will appear when
> 

I'll fix this description as you suggested. Thanks a lot!

Jae

>> +	completes memory training and testing.
>>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Julia Cartwright]

On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for PECI bus into linux
> driver framework.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
[..]
> +static int peci_locked_xfer(struct peci_adapter *adapter,
> +			    struct peci_xfer_msg *msg,
> +			    bool do_retry,
> +			    bool has_aw_fcs)

_locked generally means that this function is invoked with some critical
lock held, what lock does the caller need to acquire before invoking
this function?

> +{
> +	ktime_t start, end;
> +	s64 elapsed_ms;
> +	int rc = 0;
> +
> +	if (!adapter->xfer) {

Is this really an optional feature of an adapter?  If this is not
optional, then this check should be in place when the adapter is
registered, not here.  (And it should WARN_ON(), because it's a driver
developer error).

> +		dev_dbg(&adapter->dev, "PECI level transfers not supported\n");
> +		return -ENODEV;
> +	}
> +
> +	if (in_atomic() || irqs_disabled()) {

As Andrew mentioned, this is broken.

You don't even need a might_sleep().  The locking functions you use here
will already include a might_sleep() w/ CONFIG_DEBUG_ATOMIC_SLEEP.

> +		rt_mutex_trylock(&adapter->bus_lock);
> +		if (!rc)
> +			return -EAGAIN; /* PECI activity is ongoing */
> +	} else {
> +		rt_mutex_lock(&adapter->bus_lock);
> +	}
> +
> +	if (do_retry)
> +		start = ktime_get();
> +
> +	do {
> +		rc = adapter->xfer(adapter, msg);
> +
> +		if (!do_retry)
> +			break;
> +
> +		/* Per the PECI spec, need to retry commands that return 0x8x */
> +		if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
> +			      DEV_PECI_CC_TIMEOUT)))
> +			break;

This is pretty difficult to parse.  Can you split it into two different
conditions?

> +
> +		/* Set the retry bit to indicate a retry attempt */
> +		msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;

Are you sure this bit is to be set in the _second_ byte of tx_buf?

> +
> +		/* Recalculate the AW FCS if it has one */
> +		if (has_aw_fcs)
> +			msg->tx_buf[msg->tx_len - 1] = 0x80 ^
> +						peci_aw_fcs((u8 *)msg,
> +							    2 + msg->tx_len);
> +
> +		/* Retry for at least 250ms before returning an error */
> +		end = ktime_get();
> +		elapsed_ms = ktime_to_ms(ktime_sub(end, start));
> +		if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
> +			dev_dbg(&adapter->dev, "Timeout retrying xfer!\n");
> +			break;
> +		}
> +	} while (true);
> +
> +	rt_mutex_unlock(&adapter->bus_lock);
> +
> +	return rc;
> +}
> +
> +static int peci_xfer(struct peci_adapter *adapter, struct peci_xfer_msg *msg)
> +{
> +	return peci_locked_xfer(adapter, msg, false, false);
> +}
> +
> +static int peci_xfer_with_retries(struct peci_adapter *adapter,
> +				  struct peci_xfer_msg *msg,
> +				  bool has_aw_fcs)
> +{
> +	return peci_locked_xfer(adapter, msg, true, has_aw_fcs);
> +}
> +
> +static int peci_scan_cmd_mask(struct peci_adapter *adapter)
> +{
> +	struct peci_xfer_msg msg;
> +	u32 dib;
> +	int rc = 0;
> +
> +	/* Update command mask just once */
> +	if (adapter->cmd_mask & BIT(PECI_CMD_PING))
> +		return 0;
> +
> +	msg.addr      = PECI_BASE_ADDR;
> +	msg.tx_len    = GET_DIB_WR_LEN;
> +	msg.rx_len    = GET_DIB_RD_LEN;
> +	msg.tx_buf[0] = GET_DIB_PECI_CMD;
> +
> +	rc = peci_xfer(adapter, &msg);
> +	if (rc < 0) {
> +		dev_dbg(&adapter->dev, "PECI xfer error, rc : %d\n", rc);
> +		return rc;
> +	}
> +
> +	dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
> +	      (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
> +
> +	/* Check special case for Get DIB command */
> +	if (dib == 0x00) {
> +		dev_dbg(&adapter->dev, "DIB read as 0x00\n");
> +		return -1;
> +	}
> +
> +	if (!rc) {

You should change this to:

	if (rc) {
		dev_dbg(&adapter->dev, "Error reading DIB, rc : %d\n", rc);
		return rc;
	}

And then leave the happy path below unindented.

> +		/**
> +		 * setting up the supporting commands based on minor rev#
> +		 * see PECI Spec Table 3-1
> +		 */
> +		dib = (dib >> 8) & 0xF;
> +
> +		if (dib >= 0x1) {
> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_PKG_CFG);
> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_PKG_CFG);
> +		}
> +
> +		if (dib >= 0x2)
> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_IA_MSR);
> +
> +		if (dib >= 0x3) {
> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG_LOCAL);
> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG_LOCAL);
> +		}
> +
> +		if (dib >= 0x4)
> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG);
> +
> +		if (dib >= 0x5)
> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG);
> +
> +		if (dib >= 0x6)
> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_IA_MSR);
> +
> +		adapter->cmd_mask |= BIT(PECI_CMD_GET_TEMP);
> +		adapter->cmd_mask |= BIT(PECI_CMD_GET_DIB);
> +		adapter->cmd_mask |= BIT(PECI_CMD_PING);

These cmd_mask updates are not done with any locking in mind.  Is this
intentional?  Or: is synchronization not necessary because this is
always done during enumeration prior to exposing the adapter to users?

> +	} else {
> +		dev_dbg(&adapter->dev, "Error reading DIB, rc : %d\n", rc);
> +	}
> +
> +	return rc;
> +}
> +
> +static int peci_cmd_support(struct peci_adapter *adapter, enum peci_cmd cmd)
> +{
> +	if (!(adapter->cmd_mask & BIT(PECI_CMD_PING)) &&
> +	    peci_scan_cmd_mask(adapter) < 0) {
> +		dev_dbg(&adapter->dev, "Failed to scan command mask\n");
> +		return -EIO;
> +	}
> +
> +	if (!(adapter->cmd_mask & BIT(cmd))) {
> +		dev_dbg(&adapter->dev, "Command %d is not supported\n", cmd);
> +		return -EINVAL;
> +	}

It would be nicer if you did this check prior to dispatching to the
various subfunctions (peci_ioctl_ping, peci_ioctl_get_dib, etc.).  In
that way, these functions could just assume the adapter supports them.

[..]
> +static int peci_register_adapter(struct peci_adapter *adapter)
> +{
> +	int res = -EINVAL;
> +
> +	/* Can't register until after driver model init */
> +	if (WARN_ON(!is_registered)) {

Is this solving a problem you actually ran into?

[.. skipped review due to fatigue ..]

> +++ b/include/linux/peci.h
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Intel Corporation
> +
> +#ifndef __LINUX_PECI_H
> +#define __LINUX_PECI_H
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/peci-ioctl.h>
> +#include <linux/rtmutex.h>
> +
> +#define PECI_BUFFER_SIZE  32
> +#define PECI_NAME_SIZE    32
> +
> +struct peci_xfer_msg {
> +	u8	addr;
> +	u8	tx_len;
> +	u8	rx_len;
> +	u8	tx_buf[PECI_BUFFER_SIZE];
> +	u8	rx_buf[PECI_BUFFER_SIZE];
> +} __attribute__((__packed__));

The packed attribute has historically caused gcc to emit atrocious code,
as it seems to assume packed implies members might not be naturally
aligned.  Seeing as you're only working with u8s in this case, though,
this shouldn't be a problem.

> +struct peci_board_info {
> +	char			type[PECI_NAME_SIZE];
> +	u8			addr;	/* CPU client address */
> +	struct device_node	*of_node;
> +};
> +
> +struct peci_adapter {
> +	struct module	*owner;
> +	struct rt_mutex	bus_lock;

Why an rt_mutex, instead of a regular mutex.  Do you explicitly need PI
in mainline?

> +	struct device	dev;
> +	struct cdev	cdev;
> +	int		nr;
> +	char		name[PECI_NAME_SIZE];
> +	int		(*xfer)(struct peci_adapter *adapter,
> +				struct peci_xfer_msg *msg);
> +	uint		cmd_mask;
> +};
> +
> +#define to_peci_adapter(d) container_of(d, struct peci_adapter, dev)

You can also do this with a static inline, which provides a marginally
better error when screwed up.

   Julia

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

Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core

[Jae Hyun Yoo]

Hi Julia,

Thanks for sharing your time on reviewing it. Please see my inline answers.

Jae

On 3/6/2018 7:19 PM, Julia Cartwright wrote:
> On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:
>> This commit adds driver implementation for PECI bus into linux
>> driver framework.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
> [..]
>> +static int peci_locked_xfer(struct peci_adapter *adapter,
>> +			    struct peci_xfer_msg *msg,
>> +			    bool do_retry,
>> +			    bool has_aw_fcs)
> 
> _locked generally means that this function is invoked with some critical
> lock held, what lock does the caller need to acquire before invoking
> this function?
> 

I intended to show that this function has a mutex locking inside for 
serialization of PECI data transactions from multiple callers, but as 
you commented out below, the mutex protection scope should be adjusted 
to make that covers the peci_scan_cmd_mask() function too. I'll rewrite 
the mutex protection scope then this function will be in the locked scope.

>> +{
>> +	ktime_t start, end;
>> +	s64 elapsed_ms;
>> +	int rc = 0;
>> +
>> +	if (!adapter->xfer) {
> 
> Is this really an optional feature of an adapter?  If this is not
> optional, then this check should be in place when the adapter is
> registered, not here.  (And it should WARN_ON(), because it's a driver
> developer error).
> 

I agree with you. I'll move this code into the peci_register_adapter() 
function.

>> +		dev_dbg(&adapter->dev, "PECI level transfers not supported\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (in_atomic() || irqs_disabled()) {
> 
> As Andrew mentioned, this is broken.
> 
> You don't even need a might_sleep().  The locking functions you use here
> will already include a might_sleep() w/ CONFIG_DEBUG_ATOMIC_SLEEP.
> 

Thanks for letting me know that. I'll drop that checking code and 
might_sleep() too.

>> +		rt_mutex_trylock(&adapter->bus_lock);
>> +		if (!rc)
>> +			return -EAGAIN; /* PECI activity is ongoing */
>> +	} else {
>> +		rt_mutex_lock(&adapter->bus_lock);
>> +	}
>> +
>> +	if (do_retry)
>> +		start = ktime_get();
>> +
>> +	do {
>> +		rc = adapter->xfer(adapter, msg);
>> +
>> +		if (!do_retry)
>> +			break;
>> +
>> +		/* Per the PECI spec, need to retry commands that return 0x8x */
>> +		if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
>> +			      DEV_PECI_CC_TIMEOUT)))
>> +			break;
> 
> This is pretty difficult to parse.  Can you split it into two different
> conditions?
> 

Sure. I'll split it out.

>> +
>> +		/* Set the retry bit to indicate a retry attempt */
>> +		msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;
> 
> Are you sure this bit is to be set in the _second_ byte of tx_buf?
> 

Yes, I'm pretty sure. The first byte contains a PECI command value and 
the second byte contains 'HostID[7:1] & Retry[0]' value.

>> +
>> +		/* Recalculate the AW FCS if it has one */
>> +		if (has_aw_fcs)
>> +			msg->tx_buf[msg->tx_len - 1] = 0x80 ^
>> +						peci_aw_fcs((u8 *)msg,
>> +							    2 + msg->tx_len);
>> +
>> +		/* Retry for at least 250ms before returning an error */
>> +		end = ktime_get();
>> +		elapsed_ms = ktime_to_ms(ktime_sub(end, start));
>> +		if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
>> +			dev_dbg(&adapter->dev, "Timeout retrying xfer!\n");
>> +			break;
>> +		}
>> +	} while (true);
>> +
>> +	rt_mutex_unlock(&adapter->bus_lock);
>> +
>> +	return rc;
>> +}
>> +
>> +static int peci_xfer(struct peci_adapter *adapter, struct peci_xfer_msg *msg)
>> +{
>> +	return peci_locked_xfer(adapter, msg, false, false);
>> +}
>> +
>> +static int peci_xfer_with_retries(struct peci_adapter *adapter,
>> +				  struct peci_xfer_msg *msg,
>> +				  bool has_aw_fcs)
>> +{
>> +	return peci_locked_xfer(adapter, msg, true, has_aw_fcs);
>> +}
>> +
>> +static int peci_scan_cmd_mask(struct peci_adapter *adapter)
>> +{
>> +	struct peci_xfer_msg msg;
>> +	u32 dib;
>> +	int rc = 0;
>> +
>> +	/* Update command mask just once */
>> +	if (adapter->cmd_mask & BIT(PECI_CMD_PING))
>> +		return 0;
>> +
>> +	msg.addr      = PECI_BASE_ADDR;
>> +	msg.tx_len    = GET_DIB_WR_LEN;
>> +	msg.rx_len    = GET_DIB_RD_LEN;
>> +	msg.tx_buf[0] = GET_DIB_PECI_CMD;
>> +
>> +	rc = peci_xfer(adapter, &msg);
>> +	if (rc < 0) {
>> +		dev_dbg(&adapter->dev, "PECI xfer error, rc : %d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
>> +	      (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
>> +
>> +	/* Check special case for Get DIB command */
>> +	if (dib == 0x00) {
>> +		dev_dbg(&adapter->dev, "DIB read as 0x00\n");
>> +		return -1;
>> +	}
>> +
>> +	if (!rc) {
> 
> You should change this to:
> 
> 	if (rc) {
> 		dev_dbg(&adapter->dev, "Error reading DIB, rc : %d\n", rc);
> 		return rc;
> 	}
> 
> And then leave the happy path below unindented.
> 

Agreed. That would be neater. Will rewrite it. Thanks!

>> +		/**
>> +		 * setting up the supporting commands based on minor rev#
>> +		 * see PECI Spec Table 3-1
>> +		 */
>> +		dib = (dib >> 8) & 0xF;
>> +
>> +		if (dib >= 0x1) {
>> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_PKG_CFG);
>> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_PKG_CFG);
>> +		}
>> +
>> +		if (dib >= 0x2)
>> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_IA_MSR);
>> +
>> +		if (dib >= 0x3) {
>> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG_LOCAL);
>> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG_LOCAL);
>> +		}
>> +
>> +		if (dib >= 0x4)
>> +			adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG);
>> +
>> +		if (dib >= 0x5)
>> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG);
>> +
>> +		if (dib >= 0x6)
>> +			adapter->cmd_mask |= BIT(PECI_CMD_WR_IA_MSR);
>> +
>> +		adapter->cmd_mask |= BIT(PECI_CMD_GET_TEMP);
>> +		adapter->cmd_mask |= BIT(PECI_CMD_GET_DIB);
>> +		adapter->cmd_mask |= BIT(PECI_CMD_PING);
> 
> These cmd_mask updates are not done with any locking in mind.  Is this
> intentional?  Or: is synchronization not necessary because this is
> always done during enumeration prior to exposing the adapter to users?
> 

Thanks for the pointing it out. This function should be done in a locked 
scope as you said. I'll adjust mutex protection scope to make that 
covers this function as well.

>> +	} else {
>> +		dev_dbg(&adapter->dev, "Error reading DIB, rc : %d\n", rc);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>> +static int peci_cmd_support(struct peci_adapter *adapter, enum peci_cmd cmd)
>> +{
>> +	if (!(adapter->cmd_mask & BIT(PECI_CMD_PING)) &&
>> +	    peci_scan_cmd_mask(adapter) < 0) {
>> +		dev_dbg(&adapter->dev, "Failed to scan command mask\n");
>> +		return -EIO;
>> +	}
>> +
>> +	if (!(adapter->cmd_mask & BIT(cmd))) {
>> +		dev_dbg(&adapter->dev, "Command %d is not supported\n", cmd);
>> +		return -EINVAL;
>> +	}
> 
> It would be nicer if you did this check prior to dispatching to the
> various subfunctions (peci_ioctl_ping, peci_ioctl_get_dib, etc.).  In
> that way, these functions could just assume the adapter supports them.
> 

Agreed. I'll drop all individual calls from subfunctions and will call 
it from peci_command().

> [..]
>> +static int peci_register_adapter(struct peci_adapter *adapter)
>> +{
>> +	int res = -EINVAL;
>> +
>> +	/* Can't register until after driver model init */
>> +	if (WARN_ON(!is_registered)) {
> 
> Is this solving a problem you actually ran into?
> 

Generally, an adapter driver registration will be happened after the 
PECI bus registration because peci_init uses postcore_initcall, but in 
case of incorrect implementation of an adapter driver which uses
a preceding postcore_initcall or a core_initcall as its module init, 
then an adapter registration would be prior to bus registration. This 
code is an exceptional case handling for that to warn the incorrect 
adapter driver implementation.

> [.. skipped review due to fatigue ..]
> 
>> +++ b/include/linux/peci.h
>> @@ -0,0 +1,97 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2018 Intel Corporation
>> +
>> +#ifndef __LINUX_PECI_H
>> +#define __LINUX_PECI_H
>> +
>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/peci-ioctl.h>
>> +#include <linux/rtmutex.h>
>> +
>> +#define PECI_BUFFER_SIZE  32
>> +#define PECI_NAME_SIZE    32
>> +
>> +struct peci_xfer_msg {
>> +	u8	addr;
>> +	u8	tx_len;
>> +	u8	rx_len;
>> +	u8	tx_buf[PECI_BUFFER_SIZE];
>> +	u8	rx_buf[PECI_BUFFER_SIZE];
>> +} __attribute__((__packed__));
> 
> The packed attribute has historically caused gcc to emit atrocious code,
> as it seems to assume packed implies members might not be naturally
> aligned.  Seeing as you're only working with u8s in this case, though,
> this shouldn't be a problem.
> 

It should be a packed struct because it is also being used for CRC8 
calculation which is treating it as a contiguous byte array.

>> +struct peci_board_info {
>> +	char			type[PECI_NAME_SIZE];
>> +	u8			addr;	/* CPU client address */
>> +	struct device_node	*of_node;
>> +};
>> +
>> +struct peci_adapter {
>> +	struct module	*owner;
>> +	struct rt_mutex	bus_lock;
> 
> Why an rt_mutex, instead of a regular mutex.  Do you explicitly need PI
> in mainline?
> 

Currently this implementation has only a temperature monitoring sideband 
feature but other sideband features such as CPU error detection and 
crash dump will be implemented later, and those additional sideband 
features should have higher priority than the temperature monitoring 
feature so it is the reason why I used an rt_mutex.

>> +	struct device	dev;
>> +	struct cdev	cdev;
>> +	int		nr;
>> +	char		name[PECI_NAME_SIZE];
>> +	int		(*xfer)(struct peci_adapter *adapter,
>> +				struct peci_xfer_msg *msg);
>> +	uint		cmd_mask;
>> +};
>> +
>> +#define to_peci_adapter(d) container_of(d, struct peci_adapter, dev)
> 
> You can also do this with a static inline, which provides a marginally
> better error when screwed up.
> 

Agreed. That would be more helpful for debugging in debug build. I'll 
rewrite the macro to a static inline like below:

static inline struct peci_adapter *to_peci_adapter(void *d)
{
	return container_of(d, struct peci_adapter, dev);
}

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

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

Hi!

> >Are these SoCs x86-based?
> 
> Yes, these are ARM SoCs. Please see Andrew's answer as well.

Understood, thanks.

> >>+	Read sampling point selection. The whole period of a bit time will be
> >>+	divided into 16 time frames. This value will determine which time frame
> >>+	this controller will sample PECI signal for data read back. Usually in
> >>+	the middle of a bit time is the best.
> >
> >English? "This value will determine when this controller"?
> >
> 
> Could I change it like below?:
> 
> "This value will determine in which time frame this controller samples PECI
> signal for data read back"

I guess... I'm not native speaker, I guess this could be improved some
more.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Milton Miller II]

About  03/07/2018 04:12PM in some time zone, Pavel Machek wrote:
>Subject: Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings:
>Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
>
>Hi!
>
>> >Are these SoCs x86-based?
>> 
>> Yes, these are ARM SoCs. Please see Andrew's answer as well.
>
>Understood, thanks.
>
>> >>+	Read sampling point selection. The whole period of a bit time
>will be
>> >>+	divided into 16 time frames. This value will determine which
>time frame
>> >>+	this controller will sample PECI signal for data read back.
>Usually in
>> >>+	the middle of a bit time is the best.
>> >
>> >English? "This value will determine when this controller"?
>> >
>> 
>> Could I change it like below?:
>> 
>> "This value will determine in which time frame this controller
>samples PECI
>> signal for data read back"
>
>I guess... I'm not native speaker, I guess this could be improved
>some
>more.
>

I agree this wording is still confusing. 

The problem is that the key subject, the time of the sampling, is in the descriptive clause "in which time frame".

"This value will determine the time frame in which the controller will sample"

or perhaps phrase it as saving a specific sample from the over-clock, or a phase of the clock.

>Best regards,
>									Pavel
>
>-- 
>(english) http://www.livejournal.com/~pavelmachek
>(cesky, pictures)
>http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

milton
--
Speaking for myself not IBM.

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

Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

[Jae Hyun Yoo]

Hi Milton,

Thanks for sharing your time to review this patch. Please see my answer 
inline.

Jae

On 3/9/2018 3:41 PM, Milton Miller II wrote:
> About  03/07/2018 04:12PM in some time zone, Pavel Machek wrote:
>> Subject: Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings:
>> Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
>>
>> Hi!
>>
>>>> Are these SoCs x86-based?
>>>
>>> Yes, these are ARM SoCs. Please see Andrew's answer as well.
>>
>> Understood, thanks.
>>
>>>>> +	Read sampling point selection. The whole period of a bit time
>> will be
>>>>> +	divided into 16 time frames. This value will determine which
>> time frame
>>>>> +	this controller will sample PECI signal for data read back.
>> Usually in
>>>>> +	the middle of a bit time is the best.
>>>>
>>>> English? "This value will determine when this controller"?
>>>>
>>>
>>> Could I change it like below?:
>>>
>>> "This value will determine in which time frame this controller
>> samples PECI
>>> signal for data read back"
>>
>> I guess... I'm not native speaker, I guess this could be improved
>> some
>> more.
>>
> 
> I agree this wording is still confusing.
> 
> The problem is that the key subject, the time of the sampling, is in the descriptive clause "in which time frame".
> 
> "This value will determine the time frame in which the controller will sample"
> 
> or perhaps phrase it as saving a specific sample from the over-clock, or a phase of the clock.
> 

Yes, that looks more better. I'll change the wording as you suggested. 
Thanks a lot!

Jae

>> Best regards,
>> 									Pavel
>>
>> -- 
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
> 
> milton
> --
> Speaking for myself not IBM.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stef van Os]

Hi Jae,

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

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

BR,
Stef

On 02/21/2018 05:16 PM, Jae Hyun Yoo wrote:
> This commit adds a generic PECI hwmon client driver implementation.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>   drivers/hwmon/Kconfig      |  10 +
>   drivers/hwmon/Makefile     |   1 +
>   drivers/hwmon/peci-hwmon.c | 928 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 939 insertions(+)
>   create mode 100644 drivers/hwmon/peci-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index ef23553ff5cb..f22e0c31f597 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called nct7904.
>   
> +config SENSORS_PECI_HWMON
> +	tristate "PECI hwmon support"
> +	depends on PECI
> +	help
> +	  If you say yes here you get support for the generic PECI hwmon
> +	  driver.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called peci-hwmon.
> +
>   config SENSORS_NSA320
>   	tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors"
>   	depends on GPIOLIB && OF
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index f814b4ace138..946f54b168e5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>   obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
>   obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
>   obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
> +obj-$(CONFIG_SENSORS_PECI_HWMON)	+= peci-hwmon.o
>   obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>   obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>   obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> diff --git a/drivers/hwmon/peci-hwmon.c b/drivers/hwmon/peci-hwmon.c
> new file mode 100644
> index 000000000000..edd27744adcb
> --- /dev/null
> +++ b/drivers/hwmon/peci-hwmon.c
> @@ -0,0 +1,928 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Intel Corporation
> +
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/peci.h>
> +#include <linux/workqueue.h>
> +
> +#define DIMM_SLOT_NUMS_MAX    12  /* Max DIMM numbers (channel ranks x 2) */
> +#define CORE_NUMS_MAX         28  /* Max core numbers (max on SKX Platinum) */
> +#define TEMP_TYPE_PECI        6   /* Sensor type 6: Intel PECI */
> +
> +#define CORE_TEMP_ATTRS       5
> +#define DIMM_TEMP_ATTRS       2
> +#define ATTR_NAME_LEN         24
> +
> +#define DEFAULT_ATTR_GRP_NUMS 5
> +
> +#define UPDATE_INTERVAL_MIN   HZ
> +#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
> +
> +enum sign {
> +	POS,
> +	NEG
> +};
> +
> +struct temp_data {
> +	bool valid;
> +	s32  value;
> +	unsigned long last_updated;
> +};
> +
> +struct temp_group {
> +	struct temp_data tjmax;
> +	struct temp_data tcontrol;
> +	struct temp_data tthrottle;
> +	struct temp_data dts_margin;
> +	struct temp_data die;
> +	struct temp_data core[CORE_NUMS_MAX];
> +	struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
> +};
> +
> +struct core_temp_group {
> +	struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
> +	char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
> +	struct attribute *attrs[CORE_TEMP_ATTRS + 1];
> +	struct attribute_group attr_group;
> +};
> +
> +struct dimm_temp_group {
> +	struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
> +	char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
> +	struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
> +	struct attribute_group attr_group;
> +};
> +
> +struct peci_hwmon {
> +	struct peci_client *client;
> +	struct device *dev;
> +	struct device *hwmon_dev;
> +	struct workqueue_struct *work_queue;
> +	struct delayed_work work_handler;
> +	char name[PECI_NAME_SIZE];
> +	struct temp_group temp;
> +	u8 addr;
> +	uint cpu_no;
> +	u32 core_mask;
> +	u32 dimm_mask;
> +	const struct attribute_group *core_attr_groups[CORE_NUMS_MAX + 1];
> +	const struct attribute_group *dimm_attr_groups[DIMM_SLOT_NUMS_MAX + 1];
> +	uint global_idx;
> +	uint core_idx;
> +	uint dimm_idx;
> +};
> +
> +enum label {
> +	L_DIE,
> +	L_DTS,
> +	L_TCONTROL,
> +	L_TTHROTTLE,
> +	L_TJMAX,
> +	L_MAX
> +};
> +
> +static const char *peci_label[L_MAX] = {
> +	"Die\n",
> +	"DTS margin to Tcontrol\n",
> +	"Tcontrol\n",
> +	"Tthrottle\n",
> +	"Tjmax\n",
> +};
> +
> +static int send_peci_cmd(struct peci_hwmon *priv, enum peci_cmd cmd, void *msg)
> +{
> +	return peci_command(priv->client->adapter, cmd, msg);
> +}
> +
> +static int need_update(struct temp_data *temp)
> +{
> +	if (temp->valid &&
> +	    time_before(jiffies, temp->last_updated + UPDATE_INTERVAL_MIN))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static s32 ten_dot_six_to_millidegree(s32 x)
> +{
> +	return ((((x) ^ 0x8000) - 0x8000) * 1000 / 64);
> +}
> +
> +static int get_tjmax(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	int rc;
> +
> +	if (!priv->temp.tjmax.valid) {
> +		msg.addr = priv->addr;
> +		msg.index = MBX_INDEX_TEMP_TARGET;
> +		msg.param = 0;
> +		msg.rx_len = 4;
> +
> +		rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		priv->temp.tjmax.value = (s32)msg.pkg_config[2] * 1000;
> +		priv->temp.tjmax.valid = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_tcontrol(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 tcontrol_margin;
> +	int rc;
> +
> +	if (!need_update(&priv->temp.tcontrol))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.addr = priv->addr;
> +	msg.index = MBX_INDEX_TEMP_TARGET;
> +	msg.param = 0;
> +	msg.rx_len = 4;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	tcontrol_margin = msg.pkg_config[1];
> +	tcontrol_margin = ((tcontrol_margin ^ 0x80) - 0x80) * 1000;
> +
> +	priv->temp.tcontrol.value = priv->temp.tjmax.value - tcontrol_margin;
> +
> +	if (!priv->temp.tcontrol.valid) {
> +		priv->temp.tcontrol.last_updated = INITIAL_JIFFIES;
> +		priv->temp.tcontrol.valid = true;
> +	} else {
> +		priv->temp.tcontrol.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_tthrottle(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 tthrottle_offset;
> +	int rc;
> +
> +	if (!need_update(&priv->temp.tthrottle))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.addr = priv->addr;
> +	msg.index = MBX_INDEX_TEMP_TARGET;
> +	msg.param = 0;
> +	msg.rx_len = 4;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	tthrottle_offset = (msg.pkg_config[3] & 0x2f) * 1000;
> +	priv->temp.tthrottle.value = priv->temp.tjmax.value - tthrottle_offset;
> +
> +	if (!priv->temp.tthrottle.valid) {
> +		priv->temp.tthrottle.last_updated = INITIAL_JIFFIES;
> +		priv->temp.tthrottle.valid = true;
> +	} else {
> +		priv->temp.tthrottle.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_die_temp(struct peci_hwmon *priv)
> +{
> +	struct peci_get_temp_msg msg;
> +	int rc;
> +
> +	if (!need_update(&priv->temp.die))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.addr = priv->addr;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_GET_TEMP, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	priv->temp.die.value = priv->temp.tjmax.value +
> +			       ((s32)msg.temp_raw * 1000 / 64);
> +
> +	if (!priv->temp.die.valid) {
> +		priv->temp.die.last_updated = INITIAL_JIFFIES;
> +		priv->temp.die.valid = true;
> +	} else {
> +		priv->temp.die.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_dts_margin(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 dts_margin;
> +	int rc;
> +
> +	if (!need_update(&priv->temp.dts_margin))
> +		return 0;
> +
> +	msg.addr = priv->addr;
> +	msg.index = MBX_INDEX_DTS_MARGIN;
> +	msg.param = 0;
> +	msg.rx_len = 4;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
> +
> +	/**
> +	 * Processors return a value of DTS reading in 10.6 format
> +	 * (10 bits signed decimal, 6 bits fractional).
> +	 * Error codes:
> +	 *   0x8000: General sensor error
> +	 *   0x8001: Reserved
> +	 *   0x8002: Underflow on reading value
> +	 *   0x8003-0x81ff: Reserved
> +	 */
> +	if (dts_margin >= 0x8000 && dts_margin <= 0x81ff)
> +		return -1;
> +
> +	dts_margin = ten_dot_six_to_millidegree(dts_margin);
> +
> +	priv->temp.dts_margin.value = dts_margin;
> +
> +	if (!priv->temp.dts_margin.valid) {
> +		priv->temp.dts_margin.last_updated = INITIAL_JIFFIES;
> +		priv->temp.dts_margin.valid = true;
> +	} else {
> +		priv->temp.dts_margin.last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_core_temp(struct peci_hwmon *priv, int core_index)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	s32 core_dts_margin;
> +	int rc;
> +
> +	if (!need_update(&priv->temp.core[core_index]))
> +		return 0;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	msg.addr = priv->addr;
> +	msg.index = MBX_INDEX_PER_CORE_DTS_TEMP;
> +	msg.param = core_index;
> +	msg.rx_len = 4;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	core_dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
> +
> +	/**
> +	 * Processors return a value of the core DTS reading in 10.6 format
> +	 * (10 bits signed decimal, 6 bits fractional).
> +	 * Error codes:
> +	 *   0x8000: General sensor error
> +	 *   0x8001: Reserved
> +	 *   0x8002: Underflow on reading value
> +	 *   0x8003-0x81ff: Reserved
> +	 */
> +	if (core_dts_margin >= 0x8000 && core_dts_margin <= 0x81ff)
> +		return -1;
> +
> +	core_dts_margin = ten_dot_six_to_millidegree(core_dts_margin);
> +
> +	priv->temp.core[core_index].value = priv->temp.tjmax.value +
> +					    core_dts_margin;
> +
> +	if (!priv->temp.core[core_index].valid) {
> +		priv->temp.core[core_index].last_updated = INITIAL_JIFFIES;
> +		priv->temp.core[core_index].valid = true;
> +	} else {
> +		priv->temp.core[core_index].last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_dimm_temp(struct peci_hwmon *priv, int dimm_index)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	int channel = dimm_index / 2;
> +	int dimm_order = dimm_index % 2;
> +	int rc;
> +
> +	if (!need_update(&priv->temp.dimm[dimm_index]))
> +		return 0;
> +
> +	msg.addr = priv->addr;
> +	msg.index = MBX_INDEX_DDR_DIMM_TEMP;
> +	msg.param = channel;
> +	msg.rx_len = 4;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	priv->temp.dimm[dimm_index].value = msg.pkg_config[dimm_order] * 1000;
> +
> +	if (!priv->temp.dimm[dimm_index].valid) {
> +		priv->temp.dimm[dimm_index].last_updated = INITIAL_JIFFIES;
> +		priv->temp.dimm[dimm_index].valid = true;
> +	} else {
> +		priv->temp.dimm[dimm_index].last_updated = jiffies;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t show_tcontrol(struct device *dev,
> +			     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_tcontrol(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.tcontrol.value);
> +}
> +
> +static ssize_t show_tcontrol_margin(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int rc;
> +
> +	rc = get_tcontrol(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", sensor_attr->index == POS ?
> +				    priv->temp.tjmax.value -
> +				    priv->temp.tcontrol.value :
> +				    priv->temp.tcontrol.value -
> +				    priv->temp.tjmax.value);
> +}
> +
> +static ssize_t show_tthrottle(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_tthrottle(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.tthrottle.value);
> +}
> +
> +static ssize_t show_tjmax(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_tjmax(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.tjmax.value);
> +}
> +
> +static ssize_t show_die_temp(struct device *dev,
> +			     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_die_temp(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.die.value);
> +}
> +
> +static ssize_t show_dts_margin(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = get_dts_margin(priv);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.dts_margin.value);
> +}
> +
> +static ssize_t show_core_temp(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int core_index = sensor_attr->index;
> +	int rc;
> +
> +	rc = get_core_temp(priv, core_index);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.core[core_index].value);
> +}
> +
> +static ssize_t show_dimm_temp(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int dimm_index = sensor_attr->index;
> +	int rc;
> +
> +	rc = get_dimm_temp(priv, dimm_index);
> +	if (rc < 0)
> +		return rc;
> +
> +	return sprintf(buf, "%d\n", priv->temp.dimm[dimm_index].value);
> +}
> +
> +static ssize_t show_value(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	return sprintf(buf, "%d\n", sensor_attr->index);
> +}
> +
> +static ssize_t show_label(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	return sprintf(buf, peci_label[sensor_attr->index]);
> +}
> +
> +static ssize_t show_core_label(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	return sprintf(buf, "Core %d\n", sensor_attr->index);
> +}
> +
> +static ssize_t show_dimm_label(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +
> +	char channel = 'A' + (sensor_attr->index / 2);
> +	int index = sensor_attr->index % 2;
> +
> +	return sprintf(buf, "DIMM %d (%c%d)\n",
> +		       sensor_attr->index, channel, index);
> +}
> +
> +/* Die temperature */
> +static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, L_DIE);
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_die_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, 0444, show_tcontrol, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, 0444, show_tjmax, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0444, show_tcontrol_margin, NULL,
> +			  POS);
> +
> +static struct attribute *die_temp_attrs[] = {
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group die_temp_attr_group = {
> +	.attrs = die_temp_attrs,
> +};
> +
> +/* DTS margin temperature */
> +static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, L_DTS);
> +static SENSOR_DEVICE_ATTR(temp2_input, 0444, show_dts_margin, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_min, 0444, show_value, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_lcrit, 0444, show_tcontrol_margin, NULL, NEG);
> +
> +static struct attribute *dts_margin_temp_attrs[] = {
> +	&sensor_dev_attr_temp2_label.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_min.dev_attr.attr,
> +	&sensor_dev_attr_temp2_lcrit.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group dts_margin_temp_attr_group = {
> +	.attrs = dts_margin_temp_attrs,
> +};
> +
> +/* Tcontrol temperature */
> +static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, L_TCONTROL);
> +static SENSOR_DEVICE_ATTR(temp3_input, 0444, show_tcontrol, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp3_crit, 0444, show_tjmax, NULL, 0);
> +
> +static struct attribute *tcontrol_temp_attrs[] = {
> +	&sensor_dev_attr_temp3_label.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group tcontrol_temp_attr_group = {
> +	.attrs = tcontrol_temp_attrs,
> +};
> +
> +/* Tthrottle temperature */
> +static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, L_TTHROTTLE);
> +static SENSOR_DEVICE_ATTR(temp4_input, 0444, show_tthrottle, NULL, 0);
> +
> +static struct attribute *tthrottle_temp_attrs[] = {
> +	&sensor_dev_attr_temp4_label.dev_attr.attr,
> +	&sensor_dev_attr_temp4_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group tthrottle_temp_attr_group = {
> +	.attrs = tthrottle_temp_attrs,
> +};
> +
> +/* Tjmax temperature */
> +static SENSOR_DEVICE_ATTR(temp5_label, 0444, show_label, NULL, L_TJMAX);
> +static SENSOR_DEVICE_ATTR(temp5_input, 0444, show_tjmax, NULL, 0);
> +
> +static struct attribute *tjmax_temp_attrs[] = {
> +	&sensor_dev_attr_temp5_label.dev_attr.attr,
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group tjmax_temp_attr_group = {
> +	.attrs = tjmax_temp_attrs,
> +};
> +
> +static const struct attribute_group *
> +default_attr_groups[DEFAULT_ATTR_GRP_NUMS + 1] = {
> +	&die_temp_attr_group,
> +	&dts_margin_temp_attr_group,
> +	&tcontrol_temp_attr_group,
> +	&tthrottle_temp_attr_group,
> +	&tjmax_temp_attr_group,
> +	NULL
> +};
> +
> +/* Core temperature */
> +static ssize_t (*const core_show_fn[CORE_TEMP_ATTRS]) (struct device *dev,
> +		struct device_attribute *devattr, char *buf) = {
> +	show_core_label,
> +	show_core_temp,
> +	show_tcontrol,
> +	show_tjmax,
> +	show_tcontrol_margin,
> +};
> +
> +static const char *const core_suffix[CORE_TEMP_ATTRS] = {
> +	"label",
> +	"input",
> +	"max",
> +	"crit",
> +	"crit_hyst",
> +};
> +
> +static int check_resolved_cores(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pci_cfg_local_msg msg;
> +	int rc;
> +
> +	if (!(priv->client->adapter->cmd_mask & BIT(PECI_CMD_RD_PCI_CFG_LOCAL)))
> +		return -EINVAL;
> +
> +	/* Get the RESOLVED_CORES register value */
> +	msg.addr = priv->addr;
> +	msg.bus = 1;
> +	msg.device = 30;
> +	msg.function = 3;
> +	msg.reg = 0xB4;
> +	msg.rx_len = 4;
> +
> +	rc = send_peci_cmd(priv, PECI_CMD_RD_PCI_CFG_LOCAL, (void *)&msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	priv->core_mask = msg.pci_config[3] << 24 |
> +			  msg.pci_config[2] << 16 |
> +			  msg.pci_config[1] << 8 |
> +			  msg.pci_config[0];
> +
> +	if (!priv->core_mask)
> +		return -EAGAIN;
> +
> +	dev_dbg(priv->dev, "Scanned resolved cores: 0x%x\n", priv->core_mask);
> +	return 0;
> +}
> +
> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no)
> +{
> +	struct core_temp_group *data;
> +	int i;
> +
> +	data = devm_kzalloc(priv->dev, sizeof(struct core_temp_group),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < CORE_TEMP_ATTRS; i++) {
> +		snprintf(data->attr_name[i], ATTR_NAME_LEN,
> +			 "temp%d_%s", priv->global_idx, core_suffix[i]);
> +		sysfs_attr_init(&data->sd_attrs[i].dev_attr.attr);
> +		data->sd_attrs[i].dev_attr.attr.name = data->attr_name[i];
> +		data->sd_attrs[i].dev_attr.attr.mode = 0444;
> +		data->sd_attrs[i].dev_attr.show = core_show_fn[i];
> +		if (i == 0 || i == 1) /* label or temp */
> +			data->sd_attrs[i].index = core_no;
> +		data->attrs[i] = &data->sd_attrs[i].dev_attr.attr;
> +	}
> +
> +	data->attr_group.attrs = data->attrs;
> +	priv->core_attr_groups[priv->core_idx++] = &data->attr_group;
> +	priv->global_idx++;
> +
> +	return 0;
> +}
> +
> +static int create_core_temp_groups(struct peci_hwmon *priv)
> +{
> +	int rc, i;
> +
> +	rc = check_resolved_cores(priv);
> +	if (!rc) {
> +		for (i = 0; i < CORE_NUMS_MAX; i++) {
> +			if (priv->core_mask & BIT(i)) {
> +				rc = create_core_temp_group(priv, i);
> +				if (rc)
> +					return rc;
> +			}
> +		}
> +
> +		rc = sysfs_create_groups(&priv->hwmon_dev->kobj,
> +					 priv->core_attr_groups);
> +	}
> +
> +	return rc;
> +}
> +
> +/* DIMM temperature */
> +static ssize_t (*const dimm_show_fn[DIMM_TEMP_ATTRS]) (struct device *dev,
> +		struct device_attribute *devattr, char *buf) = {
> +	show_dimm_label,
> +	show_dimm_temp,
> +};
> +
> +static const char *const dimm_suffix[DIMM_TEMP_ATTRS] = {
> +	"label",
> +	"input",
> +};
> +
> +static int check_populated_dimms(struct peci_hwmon *priv)
> +{
> +	struct peci_rd_pkg_cfg_msg msg;
> +	int i, rc, pass = 0;
> +
> +do_scan:
> +	for (i = 0; i < (DIMM_SLOT_NUMS_MAX / 2); i++) {
> +		msg.addr = priv->addr;
> +		msg.index = MBX_INDEX_DDR_DIMM_TEMP;
> +		msg.param = i; /* channel */
> +		msg.rx_len = 4;
> +
> +		rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
> +		if (rc < 0)
> +			return rc;
> +
> +		if (msg.pkg_config[0]) /* DIMM #0 on the channel */
> +			priv->dimm_mask |= BIT(i);
> +
> +		if (msg.pkg_config[1]) /* DIMM #1 on the channel */
> +			priv->dimm_mask |= BIT(i + 1);
> +	}
> +
> +	/* Do 2-pass scanning */
> +	if (priv->dimm_mask && pass == 0) {
> +		pass++;
> +		goto do_scan;
> +	}
> +
> +	if (!priv->dimm_mask)
> +		return -EAGAIN;
> +
> +	dev_dbg(priv->dev, "Scanned populated DIMMs: 0x%x\n", priv->dimm_mask);
> +	return 0;
> +}
> +
> +static int create_dimm_temp_group(struct peci_hwmon *priv, int dimm_no)
> +{
> +	struct dimm_temp_group *data;
> +	int i;
> +
> +	data = devm_kzalloc(priv->dev, sizeof(struct dimm_temp_group),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DIMM_TEMP_ATTRS; i++) {
> +		snprintf(data->attr_name[i], ATTR_NAME_LEN,
> +			 "temp%d_%s", priv->global_idx, dimm_suffix[i]);
> +		sysfs_attr_init(&data->sd_attrs[i].dev_attr.attr);
> +		data->sd_attrs[i].dev_attr.attr.name = data->attr_name[i];
> +		data->sd_attrs[i].dev_attr.attr.mode = 0444;
> +		data->sd_attrs[i].dev_attr.show = dimm_show_fn[i];
> +		data->sd_attrs[i].index = dimm_no;
> +		data->attrs[i] = &data->sd_attrs[i].dev_attr.attr;
> +	}
> +
> +	data->attr_group.attrs = data->attrs;
> +	priv->dimm_attr_groups[priv->dimm_idx++] = &data->attr_group;
> +	priv->global_idx++;
> +
> +	return 0;
> +}
> +
> +static int create_dimm_temp_groups(struct peci_hwmon *priv)
> +{
> +	int rc, i;
> +
> +	rc = check_populated_dimms(priv);
> +	if (!rc) {
> +		for (i = 0; i < DIMM_SLOT_NUMS_MAX; i++) {
> +			if (priv->dimm_mask & BIT(i)) {
> +				rc = create_dimm_temp_group(priv, i);
> +				if (rc)
> +					return rc;
> +			}
> +		}
> +
> +		rc = sysfs_create_groups(&priv->hwmon_dev->kobj,
> +					 priv->dimm_attr_groups);
> +		if (!rc)
> +			dev_dbg(priv->dev, "Done DIMM temp group creation\n");
> +	} else if (rc == -EAGAIN) {
> +		queue_delayed_work(priv->work_queue, &priv->work_handler,
> +				   DIMM_MASK_CHECK_DELAY);
> +		dev_dbg(priv->dev, "Diferred DIMM temp group creation\n");
> +	}
> +
> +	return rc;
> +}
> +
> +static void create_dimm_temp_groups_delayed(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct peci_hwmon *priv = container_of(dwork, struct peci_hwmon,
> +					       work_handler);
> +	int rc;
> +
> +	rc = create_dimm_temp_groups(priv);
> +	if (rc && rc != -EAGAIN)
> +		dev_dbg(priv->dev, "Skipped to creat DIMM temp groups\n");
> +}
> +
> +static int peci_hwmon_probe(struct peci_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct peci_hwmon *priv;
> +	int rc;
> +
> +	if ((client->adapter->cmd_mask &
> +	    (BIT(PECI_CMD_GET_TEMP) | BIT(PECI_CMD_RD_PKG_CFG))) !=
> +	    (BIT(PECI_CMD_GET_TEMP) | BIT(PECI_CMD_RD_PKG_CFG))) {
> +		dev_err(dev, "Client doesn't support temperature monitoring\n");
> +		return -EINVAL;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, priv);
> +	priv->client = client;
> +	priv->dev = dev;
> +	priv->addr = client->addr;
> +	priv->cpu_no = priv->addr - PECI_BASE_ADDR;
> +
> +	snprintf(priv->name, PECI_NAME_SIZE, "peci_hwmon.cpu%d", priv->cpu_no);
> +
> +	priv->work_queue = create_singlethread_workqueue(priv->name);
> +	if (!priv->work_queue)
> +		return -ENOMEM;
> +
> +	priv->hwmon_dev = hwmon_device_register_with_groups(priv->dev,
> +							    priv->name,
> +							    priv,
> +							   default_attr_groups);
> +
> +	rc = PTR_ERR_OR_ZERO(priv->hwmon_dev);
> +	if (rc) {
> +		dev_err(dev, "Failed to register peci hwmon\n");
> +		return rc;
> +	}
> +
> +	priv->global_idx = DEFAULT_ATTR_GRP_NUMS + 1;
> +
> +	rc = create_core_temp_groups(priv);
> +	if (rc) {
> +		dev_err(dev, "Failed to create core groups\n");
> +		return rc;
> +	}
> +
> +	INIT_DELAYED_WORK(&priv->work_handler, create_dimm_temp_groups_delayed);
> +
> +	rc = create_dimm_temp_groups(priv);
> +	if (rc && rc != -EAGAIN)
> +		dev_dbg(dev, "Skipped to creat DIMM temp groups\n");
> +
> +	dev_dbg(dev, "peci hwmon for CPU at 0x%x registered\n", priv->addr);
> +
> +	return 0;
> +}
> +
> +static int peci_hwmon_remove(struct peci_client *client)
> +{
> +	struct peci_hwmon *priv = dev_get_drvdata(&client->dev);
> +
> +	cancel_delayed_work(&priv->work_handler);
> +	destroy_workqueue(priv->work_queue);
> +	sysfs_remove_groups(&priv->hwmon_dev->kobj, priv->core_attr_groups);
> +	sysfs_remove_groups(&priv->hwmon_dev->kobj, priv->dimm_attr_groups);
> +	hwmon_device_unregister(priv->hwmon_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id peci_of_table[] = {
> +	{ .compatible = "intel,peci-hwmon", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, peci_of_table);
> +
> +static struct peci_driver peci_hwmon_driver = {
> +	.probe  = peci_hwmon_probe,
> +	.remove = peci_hwmon_remove,
> +	.driver = {
> +		.name           = "peci-hwmon",
> +		.of_match_table = of_match_ptr(peci_of_table),
> +	},
> +};
> +module_peci_driver(peci_hwmon_driver);
> +
> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> +MODULE_DESCRIPTION("PECI hwmon driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Stef van Os
Designer
Prodrive Technologies B.V.
Mobile: +31 63 17 76 319
Phone:  +31 40 26 76 200
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jae Hyun Yoo]

Hi Stef,

Thanks for sharing your time to test it.

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

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

Thanks,
Jae

On 3/13/2018 2:32 AM, Stef van Os wrote:
> Hi Jae,
> 
> I tried version 1 and 2 of your PECI patch on our (AST2500 / Xeon E5 v4) 
> system. The V1 patchset works as expected (reading back temperature 0 
> until PECI is up), but the hwmon driver probe fails with version 2. It 
> communicates with the Xeon and assumes during kernel boot of the Aspeed 
> that PECI to the Xeon's is already up and running, but our system 
> enables the main Xeon supplies from AST2500 userspace.
> 
> If I load the hwmon driver as a module to load later on, the driver does 
> not call probe like e.g. a I2C driver on the I2C bus does. Am I using V2 
> wrongly?
> 
> BR,
> Stef
> 
> On 02/21/2018 05:16 PM, Jae Hyun Yoo wrote:
>> This commit adds a generic PECI hwmon client driver implementation.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>>   drivers/hwmon/Kconfig      |  10 +
>>   drivers/hwmon/Makefile     |   1 +
>>   drivers/hwmon/peci-hwmon.c | 928 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 939 insertions(+)
>>   create mode 100644 drivers/hwmon/peci-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index ef23553ff5cb..f22e0c31f597 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1246,6 +1246,16 @@ config SENSORS_NCT7904
>>         This driver can also be built as a module.  If so, the module
>>         will be called nct7904.
>> +config SENSORS_PECI_HWMON
>> +    tristate "PECI hwmon support"
>> +    depends on PECI
>> +    help
>> +      If you say yes here you get support for the generic PECI hwmon
>> +      driver.
>> +
>> +      This driver can also be built as a module.  If so, the module
>> +      will be called peci-hwmon.
>> +
>>   config SENSORS_NSA320
>>       tristate "ZyXEL NSA320 and compatible fan speed and temperature 
>> sensors"
>>       depends on GPIOLIB && OF
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index f814b4ace138..946f54b168e5 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_NCT7802)    += nct7802.o
>>   obj-$(CONFIG_SENSORS_NCT7904)    += nct7904.o
>>   obj-$(CONFIG_SENSORS_NSA320)    += nsa320-hwmon.o
>>   obj-$(CONFIG_SENSORS_NTC_THERMISTOR)    += ntc_thermistor.o
>> +obj-$(CONFIG_SENSORS_PECI_HWMON)    += peci-hwmon.o
>>   obj-$(CONFIG_SENSORS_PC87360)    += pc87360.o
>>   obj-$(CONFIG_SENSORS_PC87427)    += pc87427.o
>>   obj-$(CONFIG_SENSORS_PCF8591)    += pcf8591.o
>> diff --git a/drivers/hwmon/peci-hwmon.c b/drivers/hwmon/peci-hwmon.c
>> new file mode 100644
>> index 000000000000..edd27744adcb
>> --- /dev/null
>> +++ b/drivers/hwmon/peci-hwmon.c
>> @@ -0,0 +1,928 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2018 Intel Corporation
>> +
>> +#include <linux/delay.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/peci.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define DIMM_SLOT_NUMS_MAX    12  /* Max DIMM numbers (channel ranks 
>> x 2) */
>> +#define CORE_NUMS_MAX         28  /* Max core numbers (max on SKX 
>> Platinum) */
>> +#define TEMP_TYPE_PECI        6   /* Sensor type 6: Intel PECI */
>> +
>> +#define CORE_TEMP_ATTRS       5
>> +#define DIMM_TEMP_ATTRS       2
>> +#define ATTR_NAME_LEN         24
>> +
>> +#define DEFAULT_ATTR_GRP_NUMS 5
>> +
>> +#define UPDATE_INTERVAL_MIN   HZ
>> +#define DIMM_MASK_CHECK_DELAY msecs_to_jiffies(5000)
>> +
>> +enum sign {
>> +    POS,
>> +    NEG
>> +};
>> +
>> +struct temp_data {
>> +    bool valid;
>> +    s32  value;
>> +    unsigned long last_updated;
>> +};
>> +
>> +struct temp_group {
>> +    struct temp_data tjmax;
>> +    struct temp_data tcontrol;
>> +    struct temp_data tthrottle;
>> +    struct temp_data dts_margin;
>> +    struct temp_data die;
>> +    struct temp_data core[CORE_NUMS_MAX];
>> +    struct temp_data dimm[DIMM_SLOT_NUMS_MAX];
>> +};
>> +
>> +struct core_temp_group {
>> +    struct sensor_device_attribute sd_attrs[CORE_TEMP_ATTRS];
>> +    char attr_name[CORE_TEMP_ATTRS][ATTR_NAME_LEN];
>> +    struct attribute *attrs[CORE_TEMP_ATTRS + 1];
>> +    struct attribute_group attr_group;
>> +};
>> +
>> +struct dimm_temp_group {
>> +    struct sensor_device_attribute sd_attrs[DIMM_TEMP_ATTRS];
>> +    char attr_name[DIMM_TEMP_ATTRS][ATTR_NAME_LEN];
>> +    struct attribute *attrs[DIMM_TEMP_ATTRS + 1];
>> +    struct attribute_group attr_group;
>> +};
>> +
>> +struct peci_hwmon {
>> +    struct peci_client *client;
>> +    struct device *dev;
>> +    struct device *hwmon_dev;
>> +    struct workqueue_struct *work_queue;
>> +    struct delayed_work work_handler;
>> +    char name[PECI_NAME_SIZE];
>> +    struct temp_group temp;
>> +    u8 addr;
>> +    uint cpu_no;
>> +    u32 core_mask;
>> +    u32 dimm_mask;
>> +    const struct attribute_group *core_attr_groups[CORE_NUMS_MAX + 1];
>> +    const struct attribute_group *dimm_attr_groups[DIMM_SLOT_NUMS_MAX 
>> + 1];
>> +    uint global_idx;
>> +    uint core_idx;
>> +    uint dimm_idx;
>> +};
>> +
>> +enum label {
>> +    L_DIE,
>> +    L_DTS,
>> +    L_TCONTROL,
>> +    L_TTHROTTLE,
>> +    L_TJMAX,
>> +    L_MAX
>> +};
>> +
>> +static const char *peci_label[L_MAX] = {
>> +    "Die\n",
>> +    "DTS margin to Tcontrol\n",
>> +    "Tcontrol\n",
>> +    "Tthrottle\n",
>> +    "Tjmax\n",
>> +};
>> +
>> +static int send_peci_cmd(struct peci_hwmon *priv, enum peci_cmd cmd, 
>> void *msg)
>> +{
>> +    return peci_command(priv->client->adapter, cmd, msg);
>> +}
>> +
>> +static int need_update(struct temp_data *temp)
>> +{
>> +    if (temp->valid &&
>> +        time_before(jiffies, temp->last_updated + UPDATE_INTERVAL_MIN))
>> +        return 0;
>> +
>> +    return 1;
>> +}
>> +
>> +static s32 ten_dot_six_to_millidegree(s32 x)
>> +{
>> +    return ((((x) ^ 0x8000) - 0x8000) * 1000 / 64);
>> +}
>> +
>> +static int get_tjmax(struct peci_hwmon *priv)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    int rc;
>> +
>> +    if (!priv->temp.tjmax.valid) {
>> +        msg.addr = priv->addr;
>> +        msg.index = MBX_INDEX_TEMP_TARGET;
>> +        msg.param = 0;
>> +        msg.rx_len = 4;
>> +
>> +        rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +        if (rc < 0)
>> +            return rc;
>> +
>> +        priv->temp.tjmax.value = (s32)msg.pkg_config[2] * 1000;
>> +        priv->temp.tjmax.valid = true;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_tcontrol(struct peci_hwmon *priv)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    s32 tcontrol_margin;
>> +    int rc;
>> +
>> +    if (!need_update(&priv->temp.tcontrol))
>> +        return 0;
>> +
>> +    rc = get_tjmax(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    msg.addr = priv->addr;
>> +    msg.index = MBX_INDEX_TEMP_TARGET;
>> +    msg.param = 0;
>> +    msg.rx_len = 4;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    tcontrol_margin = msg.pkg_config[1];
>> +    tcontrol_margin = ((tcontrol_margin ^ 0x80) - 0x80) * 1000;
>> +
>> +    priv->temp.tcontrol.value = priv->temp.tjmax.value - 
>> tcontrol_margin;
>> +
>> +    if (!priv->temp.tcontrol.valid) {
>> +        priv->temp.tcontrol.last_updated = INITIAL_JIFFIES;
>> +        priv->temp.tcontrol.valid = true;
>> +    } else {
>> +        priv->temp.tcontrol.last_updated = jiffies;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_tthrottle(struct peci_hwmon *priv)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    s32 tthrottle_offset;
>> +    int rc;
>> +
>> +    if (!need_update(&priv->temp.tthrottle))
>> +        return 0;
>> +
>> +    rc = get_tjmax(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    msg.addr = priv->addr;
>> +    msg.index = MBX_INDEX_TEMP_TARGET;
>> +    msg.param = 0;
>> +    msg.rx_len = 4;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    tthrottle_offset = (msg.pkg_config[3] & 0x2f) * 1000;
>> +    priv->temp.tthrottle.value = priv->temp.tjmax.value - 
>> tthrottle_offset;
>> +
>> +    if (!priv->temp.tthrottle.valid) {
>> +        priv->temp.tthrottle.last_updated = INITIAL_JIFFIES;
>> +        priv->temp.tthrottle.valid = true;
>> +    } else {
>> +        priv->temp.tthrottle.last_updated = jiffies;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_die_temp(struct peci_hwmon *priv)
>> +{
>> +    struct peci_get_temp_msg msg;
>> +    int rc;
>> +
>> +    if (!need_update(&priv->temp.die))
>> +        return 0;
>> +
>> +    rc = get_tjmax(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    msg.addr = priv->addr;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_GET_TEMP, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    priv->temp.die.value = priv->temp.tjmax.value +
>> +                   ((s32)msg.temp_raw * 1000 / 64);
>> +
>> +    if (!priv->temp.die.valid) {
>> +        priv->temp.die.last_updated = INITIAL_JIFFIES;
>> +        priv->temp.die.valid = true;
>> +    } else {
>> +        priv->temp.die.last_updated = jiffies;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_dts_margin(struct peci_hwmon *priv)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    s32 dts_margin;
>> +    int rc;
>> +
>> +    if (!need_update(&priv->temp.dts_margin))
>> +        return 0;
>> +
>> +    msg.addr = priv->addr;
>> +    msg.index = MBX_INDEX_DTS_MARGIN;
>> +    msg.param = 0;
>> +    msg.rx_len = 4;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
>> +
>> +    /**
>> +     * Processors return a value of DTS reading in 10.6 format
>> +     * (10 bits signed decimal, 6 bits fractional).
>> +     * Error codes:
>> +     *   0x8000: General sensor error
>> +     *   0x8001: Reserved
>> +     *   0x8002: Underflow on reading value
>> +     *   0x8003-0x81ff: Reserved
>> +     */
>> +    if (dts_margin >= 0x8000 && dts_margin <= 0x81ff)
>> +        return -1;
>> +
>> +    dts_margin = ten_dot_six_to_millidegree(dts_margin);
>> +
>> +    priv->temp.dts_margin.value = dts_margin;
>> +
>> +    if (!priv->temp.dts_margin.valid) {
>> +        priv->temp.dts_margin.last_updated = INITIAL_JIFFIES;
>> +        priv->temp.dts_margin.valid = true;
>> +    } else {
>> +        priv->temp.dts_margin.last_updated = jiffies;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_core_temp(struct peci_hwmon *priv, int core_index)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    s32 core_dts_margin;
>> +    int rc;
>> +
>> +    if (!need_update(&priv->temp.core[core_index]))
>> +        return 0;
>> +
>> +    rc = get_tjmax(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    msg.addr = priv->addr;
>> +    msg.index = MBX_INDEX_PER_CORE_DTS_TEMP;
>> +    msg.param = core_index;
>> +    msg.rx_len = 4;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    core_dts_margin = (msg.pkg_config[1] << 8) | msg.pkg_config[0];
>> +
>> +    /**
>> +     * Processors return a value of the core DTS reading in 10.6 format
>> +     * (10 bits signed decimal, 6 bits fractional).
>> +     * Error codes:
>> +     *   0x8000: General sensor error
>> +     *   0x8001: Reserved
>> +     *   0x8002: Underflow on reading value
>> +     *   0x8003-0x81ff: Reserved
>> +     */
>> +    if (core_dts_margin >= 0x8000 && core_dts_margin <= 0x81ff)
>> +        return -1;
>> +
>> +    core_dts_margin = ten_dot_six_to_millidegree(core_dts_margin);
>> +
>> +    priv->temp.core[core_index].value = priv->temp.tjmax.value +
>> +                        core_dts_margin;
>> +
>> +    if (!priv->temp.core[core_index].valid) {
>> +        priv->temp.core[core_index].last_updated = INITIAL_JIFFIES;
>> +        priv->temp.core[core_index].valid = true;
>> +    } else {
>> +        priv->temp.core[core_index].last_updated = jiffies;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int get_dimm_temp(struct peci_hwmon *priv, int dimm_index)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    int channel = dimm_index / 2;
>> +    int dimm_order = dimm_index % 2;
>> +    int rc;
>> +
>> +    if (!need_update(&priv->temp.dimm[dimm_index]))
>> +        return 0;
>> +
>> +    msg.addr = priv->addr;
>> +    msg.index = MBX_INDEX_DDR_DIMM_TEMP;
>> +    msg.param = channel;
>> +    msg.rx_len = 4;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    priv->temp.dimm[dimm_index].value = msg.pkg_config[dimm_order] * 
>> 1000;
>> +
>> +    if (!priv->temp.dimm[dimm_index].valid) {
>> +        priv->temp.dimm[dimm_index].last_updated = INITIAL_JIFFIES;
>> +        priv->temp.dimm[dimm_index].valid = true;
>> +    } else {
>> +        priv->temp.dimm[dimm_index].last_updated = jiffies;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static ssize_t show_tcontrol(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    int rc;
>> +
>> +    rc = get_tcontrol(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.tcontrol.value);
>> +}
>> +
>> +static ssize_t show_tcontrol_margin(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +    int rc;
>> +
>> +    rc = get_tcontrol(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", sensor_attr->index == POS ?
>> +                    priv->temp.tjmax.value -
>> +                    priv->temp.tcontrol.value :
>> +                    priv->temp.tcontrol.value -
>> +                    priv->temp.tjmax.value);
>> +}
>> +
>> +static ssize_t show_tthrottle(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    int rc;
>> +
>> +    rc = get_tthrottle(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.tthrottle.value);
>> +}
>> +
>> +static ssize_t show_tjmax(struct device *dev,
>> +              struct device_attribute *attr,
>> +              char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    int rc;
>> +
>> +    rc = get_tjmax(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.tjmax.value);
>> +}
>> +
>> +static ssize_t show_die_temp(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    int rc;
>> +
>> +    rc = get_die_temp(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.die.value);
>> +}
>> +
>> +static ssize_t show_dts_margin(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    int rc;
>> +
>> +    rc = get_dts_margin(priv);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.dts_margin.value);
>> +}
>> +
>> +static ssize_t show_core_temp(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +    int core_index = sensor_attr->index;
>> +    int rc;
>> +
>> +    rc = get_core_temp(priv, core_index);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.core[core_index].value);
>> +}
>> +
>> +static ssize_t show_dimm_temp(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(dev);
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +    int dimm_index = sensor_attr->index;
>> +    int rc;
>> +
>> +    rc = get_dimm_temp(priv, dimm_index);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    return sprintf(buf, "%d\n", priv->temp.dimm[dimm_index].value);
>> +}
>> +
>> +static ssize_t show_value(struct device *dev,
>> +              struct device_attribute *attr,
>> +              char *buf)
>> +{
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +
>> +    return sprintf(buf, "%d\n", sensor_attr->index);
>> +}
>> +
>> +static ssize_t show_label(struct device *dev,
>> +              struct device_attribute *attr,
>> +              char *buf)
>> +{
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +
>> +    return sprintf(buf, peci_label[sensor_attr->index]);
>> +}
>> +
>> +static ssize_t show_core_label(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +
>> +    return sprintf(buf, "Core %d\n", sensor_attr->index);
>> +}
>> +
>> +static ssize_t show_dimm_label(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    struct sensor_device_attribute *sensor_attr = 
>> to_sensor_dev_attr(attr);
>> +
>> +    char channel = 'A' + (sensor_attr->index / 2);
>> +    int index = sensor_attr->index % 2;
>> +
>> +    return sprintf(buf, "DIMM %d (%c%d)\n",
>> +               sensor_attr->index, channel, index);
>> +}
>> +
>> +/* Die temperature */
>> +static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, L_DIE);
>> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_die_temp, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max, 0444, show_tcontrol, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit, 0444, show_tjmax, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0444, 
>> show_tcontrol_margin, NULL,
>> +              POS);
>> +
>> +static struct attribute *die_temp_attrs[] = {
>> +    &sensor_dev_attr_temp1_label.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_max.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_crit.dev_attr.attr,
>> +    &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static struct attribute_group die_temp_attr_group = {
>> +    .attrs = die_temp_attrs,
>> +};
>> +
>> +/* DTS margin temperature */
>> +static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, L_DTS);
>> +static SENSOR_DEVICE_ATTR(temp2_input, 0444, show_dts_margin, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp2_min, 0444, show_value, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp2_lcrit, 0444, show_tcontrol_margin, 
>> NULL, NEG);
>> +
>> +static struct attribute *dts_margin_temp_attrs[] = {
>> +    &sensor_dev_attr_temp2_label.dev_attr.attr,
>> +    &sensor_dev_attr_temp2_input.dev_attr.attr,
>> +    &sensor_dev_attr_temp2_min.dev_attr.attr,
>> +    &sensor_dev_attr_temp2_lcrit.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static struct attribute_group dts_margin_temp_attr_group = {
>> +    .attrs = dts_margin_temp_attrs,
>> +};
>> +
>> +/* Tcontrol temperature */
>> +static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, 
>> L_TCONTROL);
>> +static SENSOR_DEVICE_ATTR(temp3_input, 0444, show_tcontrol, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp3_crit, 0444, show_tjmax, NULL, 0);
>> +
>> +static struct attribute *tcontrol_temp_attrs[] = {
>> +    &sensor_dev_attr_temp3_label.dev_attr.attr,
>> +    &sensor_dev_attr_temp3_input.dev_attr.attr,
>> +    &sensor_dev_attr_temp3_crit.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static struct attribute_group tcontrol_temp_attr_group = {
>> +    .attrs = tcontrol_temp_attrs,
>> +};
>> +
>> +/* Tthrottle temperature */
>> +static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, 
>> L_TTHROTTLE);
>> +static SENSOR_DEVICE_ATTR(temp4_input, 0444, show_tthrottle, NULL, 0);
>> +
>> +static struct attribute *tthrottle_temp_attrs[] = {
>> +    &sensor_dev_attr_temp4_label.dev_attr.attr,
>> +    &sensor_dev_attr_temp4_input.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static struct attribute_group tthrottle_temp_attr_group = {
>> +    .attrs = tthrottle_temp_attrs,
>> +};
>> +
>> +/* Tjmax temperature */
>> +static SENSOR_DEVICE_ATTR(temp5_label, 0444, show_label, NULL, L_TJMAX);
>> +static SENSOR_DEVICE_ATTR(temp5_input, 0444, show_tjmax, NULL, 0);
>> +
>> +static struct attribute *tjmax_temp_attrs[] = {
>> +    &sensor_dev_attr_temp5_label.dev_attr.attr,
>> +    &sensor_dev_attr_temp5_input.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static struct attribute_group tjmax_temp_attr_group = {
>> +    .attrs = tjmax_temp_attrs,
>> +};
>> +
>> +static const struct attribute_group *
>> +default_attr_groups[DEFAULT_ATTR_GRP_NUMS + 1] = {
>> +    &die_temp_attr_group,
>> +    &dts_margin_temp_attr_group,
>> +    &tcontrol_temp_attr_group,
>> +    &tthrottle_temp_attr_group,
>> +    &tjmax_temp_attr_group,
>> +    NULL
>> +};
>> +
>> +/* Core temperature */
>> +static ssize_t (*const core_show_fn[CORE_TEMP_ATTRS]) (struct device 
>> *dev,
>> +        struct device_attribute *devattr, char *buf) = {
>> +    show_core_label,
>> +    show_core_temp,
>> +    show_tcontrol,
>> +    show_tjmax,
>> +    show_tcontrol_margin,
>> +};
>> +
>> +static const char *const core_suffix[CORE_TEMP_ATTRS] = {
>> +    "label",
>> +    "input",
>> +    "max",
>> +    "crit",
>> +    "crit_hyst",
>> +};
>> +
>> +static int check_resolved_cores(struct peci_hwmon *priv)
>> +{
>> +    struct peci_rd_pci_cfg_local_msg msg;
>> +    int rc;
>> +
>> +    if (!(priv->client->adapter->cmd_mask & 
>> BIT(PECI_CMD_RD_PCI_CFG_LOCAL)))
>> +        return -EINVAL;
>> +
>> +    /* Get the RESOLVED_CORES register value */
>> +    msg.addr = priv->addr;
>> +    msg.bus = 1;
>> +    msg.device = 30;
>> +    msg.function = 3;
>> +    msg.reg = 0xB4;
>> +    msg.rx_len = 4;
>> +
>> +    rc = send_peci_cmd(priv, PECI_CMD_RD_PCI_CFG_LOCAL, (void *)&msg);
>> +    if (rc < 0)
>> +        return rc;
>> +
>> +    priv->core_mask = msg.pci_config[3] << 24 |
>> +              msg.pci_config[2] << 16 |
>> +              msg.pci_config[1] << 8 |
>> +              msg.pci_config[0];
>> +
>> +    if (!priv->core_mask)
>> +        return -EAGAIN;
>> +
>> +    dev_dbg(priv->dev, "Scanned resolved cores: 0x%x\n", 
>> priv->core_mask);
>> +    return 0;
>> +}
>> +
>> +static int create_core_temp_group(struct peci_hwmon *priv, int core_no)
>> +{
>> +    struct core_temp_group *data;
>> +    int i;
>> +
>> +    data = devm_kzalloc(priv->dev, sizeof(struct core_temp_group),
>> +                GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < CORE_TEMP_ATTRS; i++) {
>> +        snprintf(data->attr_name[i], ATTR_NAME_LEN,
>> +             "temp%d_%s", priv->global_idx, core_suffix[i]);
>> +        sysfs_attr_init(&data->sd_attrs[i].dev_attr.attr);
>> +        data->sd_attrs[i].dev_attr.attr.name = data->attr_name[i];
>> +        data->sd_attrs[i].dev_attr.attr.mode = 0444;
>> +        data->sd_attrs[i].dev_attr.show = core_show_fn[i];
>> +        if (i == 0 || i == 1) /* label or temp */
>> +            data->sd_attrs[i].index = core_no;
>> +        data->attrs[i] = &data->sd_attrs[i].dev_attr.attr;
>> +    }
>> +
>> +    data->attr_group.attrs = data->attrs;
>> +    priv->core_attr_groups[priv->core_idx++] = &data->attr_group;
>> +    priv->global_idx++;
>> +
>> +    return 0;
>> +}
>> +
>> +static int create_core_temp_groups(struct peci_hwmon *priv)
>> +{
>> +    int rc, i;
>> +
>> +    rc = check_resolved_cores(priv);
>> +    if (!rc) {
>> +        for (i = 0; i < CORE_NUMS_MAX; i++) {
>> +            if (priv->core_mask & BIT(i)) {
>> +                rc = create_core_temp_group(priv, i);
>> +                if (rc)
>> +                    return rc;
>> +            }
>> +        }
>> +
>> +        rc = sysfs_create_groups(&priv->hwmon_dev->kobj,
>> +                     priv->core_attr_groups);
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +/* DIMM temperature */
>> +static ssize_t (*const dimm_show_fn[DIMM_TEMP_ATTRS]) (struct device 
>> *dev,
>> +        struct device_attribute *devattr, char *buf) = {
>> +    show_dimm_label,
>> +    show_dimm_temp,
>> +};
>> +
>> +static const char *const dimm_suffix[DIMM_TEMP_ATTRS] = {
>> +    "label",
>> +    "input",
>> +};
>> +
>> +static int check_populated_dimms(struct peci_hwmon *priv)
>> +{
>> +    struct peci_rd_pkg_cfg_msg msg;
>> +    int i, rc, pass = 0;
>> +
>> +do_scan:
>> +    for (i = 0; i < (DIMM_SLOT_NUMS_MAX / 2); i++) {
>> +        msg.addr = priv->addr;
>> +        msg.index = MBX_INDEX_DDR_DIMM_TEMP;
>> +        msg.param = i; /* channel */
>> +        msg.rx_len = 4;
>> +
>> +        rc = send_peci_cmd(priv, PECI_CMD_RD_PKG_CFG, (void *)&msg);
>> +        if (rc < 0)
>> +            return rc;
>> +
>> +        if (msg.pkg_config[0]) /* DIMM #0 on the channel */
>> +            priv->dimm_mask |= BIT(i);
>> +
>> +        if (msg.pkg_config[1]) /* DIMM #1 on the channel */
>> +            priv->dimm_mask |= BIT(i + 1);
>> +    }
>> +
>> +    /* Do 2-pass scanning */
>> +    if (priv->dimm_mask && pass == 0) {
>> +        pass++;
>> +        goto do_scan;
>> +    }
>> +
>> +    if (!priv->dimm_mask)
>> +        return -EAGAIN;
>> +
>> +    dev_dbg(priv->dev, "Scanned populated DIMMs: 0x%x\n", 
>> priv->dimm_mask);
>> +    return 0;
>> +}
>> +
>> +static int create_dimm_temp_group(struct peci_hwmon *priv, int dimm_no)
>> +{
>> +    struct dimm_temp_group *data;
>> +    int i;
>> +
>> +    data = devm_kzalloc(priv->dev, sizeof(struct dimm_temp_group),
>> +                GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < DIMM_TEMP_ATTRS; i++) {
>> +        snprintf(data->attr_name[i], ATTR_NAME_LEN,
>> +             "temp%d_%s", priv->global_idx, dimm_suffix[i]);
>> +        sysfs_attr_init(&data->sd_attrs[i].dev_attr.attr);
>> +        data->sd_attrs[i].dev_attr.attr.name = data->attr_name[i];
>> +        data->sd_attrs[i].dev_attr.attr.mode = 0444;
>> +        data->sd_attrs[i].dev_attr.show = dimm_show_fn[i];
>> +        data->sd_attrs[i].index = dimm_no;
>> +        data->attrs[i] = &data->sd_attrs[i].dev_attr.attr;
>> +    }
>> +
>> +    data->attr_group.attrs = data->attrs;
>> +    priv->dimm_attr_groups[priv->dimm_idx++] = &data->attr_group;
>> +    priv->global_idx++;
>> +
>> +    return 0;
>> +}
>> +
>> +static int create_dimm_temp_groups(struct peci_hwmon *priv)
>> +{
>> +    int rc, i;
>> +
>> +    rc = check_populated_dimms(priv);
>> +    if (!rc) {
>> +        for (i = 0; i < DIMM_SLOT_NUMS_MAX; i++) {
>> +            if (priv->dimm_mask & BIT(i)) {
>> +                rc = create_dimm_temp_group(priv, i);
>> +                if (rc)
>> +                    return rc;
>> +            }
>> +        }
>> +
>> +        rc = sysfs_create_groups(&priv->hwmon_dev->kobj,
>> +                     priv->dimm_attr_groups);
>> +        if (!rc)
>> +            dev_dbg(priv->dev, "Done DIMM temp group creation\n");
>> +    } else if (rc == -EAGAIN) {
>> +        queue_delayed_work(priv->work_queue, &priv->work_handler,
>> +                   DIMM_MASK_CHECK_DELAY);
>> +        dev_dbg(priv->dev, "Diferred DIMM temp group creation\n");
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +static void create_dimm_temp_groups_delayed(struct work_struct *work)
>> +{
>> +    struct delayed_work *dwork = to_delayed_work(work);
>> +    struct peci_hwmon *priv = container_of(dwork, struct peci_hwmon,
>> +                           work_handler);
>> +    int rc;
>> +
>> +    rc = create_dimm_temp_groups(priv);
>> +    if (rc && rc != -EAGAIN)
>> +        dev_dbg(priv->dev, "Skipped to creat DIMM temp groups\n");
>> +}
>> +
>> +static int peci_hwmon_probe(struct peci_client *client)
>> +{
>> +    struct device *dev = &client->dev;
>> +    struct peci_hwmon *priv;
>> +    int rc;
>> +
>> +    if ((client->adapter->cmd_mask &
>> +        (BIT(PECI_CMD_GET_TEMP) | BIT(PECI_CMD_RD_PKG_CFG))) !=
>> +        (BIT(PECI_CMD_GET_TEMP) | BIT(PECI_CMD_RD_PKG_CFG))) {
>> +        dev_err(dev, "Client doesn't support temperature monitoring\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    dev_set_drvdata(dev, priv);
>> +    priv->client = client;
>> +    priv->dev = dev;
>> +    priv->addr = client->addr;
>> +    priv->cpu_no = priv->addr - PECI_BASE_ADDR;
>> +
>> +    snprintf(priv->name, PECI_NAME_SIZE, "peci_hwmon.cpu%d", 
>> priv->cpu_no);
>> +
>> +    priv->work_queue = create_singlethread_workqueue(priv->name);
>> +    if (!priv->work_queue)
>> +        return -ENOMEM;
>> +
>> +    priv->hwmon_dev = hwmon_device_register_with_groups(priv->dev,
>> +                                priv->name,
>> +                                priv,
>> +                               default_attr_groups);
>> +
>> +    rc = PTR_ERR_OR_ZERO(priv->hwmon_dev);
>> +    if (rc) {
>> +        dev_err(dev, "Failed to register peci hwmon\n");
>> +        return rc;
>> +    }
>> +
>> +    priv->global_idx = DEFAULT_ATTR_GRP_NUMS + 1;
>> +
>> +    rc = create_core_temp_groups(priv);
>> +    if (rc) {
>> +        dev_err(dev, "Failed to create core groups\n");
>> +        return rc;
>> +    }
>> +
>> +    INIT_DELAYED_WORK(&priv->work_handler, 
>> create_dimm_temp_groups_delayed);
>> +
>> +    rc = create_dimm_temp_groups(priv);
>> +    if (rc && rc != -EAGAIN)
>> +        dev_dbg(dev, "Skipped to creat DIMM temp groups\n");
>> +
>> +    dev_dbg(dev, "peci hwmon for CPU at 0x%x registered\n", priv->addr);
>> +
>> +    return 0;
>> +}
>> +
>> +static int peci_hwmon_remove(struct peci_client *client)
>> +{
>> +    struct peci_hwmon *priv = dev_get_drvdata(&client->dev);
>> +
>> +    cancel_delayed_work(&priv->work_handler);
>> +    destroy_workqueue(priv->work_queue);
>> +    sysfs_remove_groups(&priv->hwmon_dev->kobj, priv->core_attr_groups);
>> +    sysfs_remove_groups(&priv->hwmon_dev->kobj, priv->dimm_attr_groups);
>> +    hwmon_device_unregister(priv->hwmon_dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id peci_of_table[] = {
>> +    { .compatible = "intel,peci-hwmon", },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(of, peci_of_table);
>> +
>> +static struct peci_driver peci_hwmon_driver = {
>> +    .probe  = peci_hwmon_probe,
>> +    .remove = peci_hwmon_remove,
>> +    .driver = {
>> +        .name           = "peci-hwmon",
>> +        .of_match_table = of_match_ptr(peci_of_table),
>> +    },
>> +};
>> +module_peci_driver(peci_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
>> +MODULE_DESCRIPTION("PECI hwmon driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html