linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support
@ 2025-08-10  8:43 Chuande Chen
  2025-08-10 23:48 ` Guenter Roeck
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Chuande Chen @ 2025-08-10  8:43 UTC (permalink / raw)
  To: jdelvare; +Cc: linux, linux-hwmon, linux-kernel, chuachen, chenchuande

From: Chuande Chen <chuachen@cisco.com>

Many AMD CPUs can support this feature now.
We would get a wrong CPU DIE temp if don't consider this.
In low-temperature environments, the CPU die temperature
can drop below zero.
So many platform would like to make extended temperature range
as their default configuration.
Default temperature range (0C to 255.875C) degree celsius.
Extended temperature range (-49C to +206.875C) degree celsius.
Ref Doc: AMD V3000 PPR (Doc ID #56558).

Signed-off-by: Chuande Chen <chuachen@cisco.com>
---
 drivers/hwmon/sbtsi_temp.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
index 3c839f56c..15e49c2a0 100644
--- a/drivers/hwmon/sbtsi_temp.c
+++ b/drivers/hwmon/sbtsi_temp.c
@@ -30,6 +30,14 @@
 #define SBTSI_REG_TEMP_LOW_DEC		0x14 /* RW */
 
 #define SBTSI_CONFIG_READ_ORDER_SHIFT	5
+/*
+ * Bit for temperature measurement range.
+ * Value=0: Use default temperature range (0C to 255.875C) for reporting temperature.
+ * Value=1: Use extended temperature range (-49C to +206.875C) for reporting temperature.
+ */
+#define SBTSI_CONFIG_EXT_RAGE_SHIFT	2
+
+#define SBTSI_TEMP_EXT_RAGE_ADJ	49000
 
 #define SBTSI_TEMP_MIN	0
 #define SBTSI_TEMP_MAX	255875
@@ -74,7 +82,12 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct sbtsi_data *data = dev_get_drvdata(dev);
 	s32 temp_int, temp_dec;
-	int err;
+	int err, cfg;
+
+	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
+	if (err < 0)
+		return err;
+	cfg = err;
 
 	switch (attr) {
 	case hwmon_temp_input:
@@ -85,12 +98,8 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 		 * so integer part should be read first. If bit == 1, read
 		 * order should be reversed.
 		 */
-		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
-		if (err < 0)
-			return err;
-
 		mutex_lock(&data->lock);
-		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
+		if (cfg & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
 			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
 			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
 		} else {
@@ -122,6 +131,8 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 		return temp_dec;
 
 	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
+	if (cfg & BIT(SBTSI_CONFIG_EXT_RAGE_SHIFT))
+		*val -= SBTSI_TEMP_EXT_RAGE_ADJ;
 
 	return 0;
 }
@@ -130,9 +141,14 @@ static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel, long val)
 {
 	struct sbtsi_data *data = dev_get_drvdata(dev);
-	int reg_int, reg_dec, err;
+	int reg_int, reg_dec, err, cfg;
 	u8 temp_int, temp_dec;
 
+	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
+	if (err < 0)
+		return err;
+	cfg = err;
+
 	switch (attr) {
 	case hwmon_temp_max:
 		reg_int = SBTSI_REG_TEMP_HIGH_INT;
@@ -146,6 +162,8 @@ static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
 		return -EINVAL;
 	}
 
+	if (cfg & BIT(SBTSI_CONFIG_EXT_RAGE_SHIFT))
+		val += SBTSI_TEMP_EXT_RAGE_ADJ;
 	val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
 	sbtsi_mc_to_reg(val, &temp_int, &temp_dec);
 
-- 
2.39.5 (Apple Git-154)


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-10  8:43 [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support Chuande Chen
@ 2025-08-10 23:48 ` Guenter Roeck
  2025-08-11  5:07 ` [PATCH v2 0/1] " Chuande Chen
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-08-10 23:48 UTC (permalink / raw)
  To: Chuande Chen, jdelvare; +Cc: linux-hwmon, linux-kernel, chuachen

On 8/10/25 01:43, Chuande Chen wrote:
> From: Chuande Chen <chuachen@cisco.com>
> 
> Many AMD CPUs can support this feature now.
> We would get a wrong CPU DIE temp if don't consider this.
> In low-temperature environments, the CPU die temperature
> can drop below zero.
> So many platform would like to make extended temperature range
> as their default configuration.
> Default temperature range (0C to 255.875C) degree celsius.
> Extended temperature range (-49C to +206.875C) degree celsius.
> Ref Doc: AMD V3000 PPR (Doc ID #56558).
> 
> Signed-off-by: Chuande Chen <chuachen@cisco.com>
> ---
>   drivers/hwmon/sbtsi_temp.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index 3c839f56c..15e49c2a0 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -30,6 +30,14 @@
>   #define SBTSI_REG_TEMP_LOW_DEC		0x14 /* RW */
>   
>   #define SBTSI_CONFIG_READ_ORDER_SHIFT	5
> +/*
> + * Bit for temperature measurement range.
> + * Value=0: Use default temperature range (0C to 255.875C) for reporting temperature.
> + * Value=1: Use extended temperature range (-49C to +206.875C) for reporting temperature.
> + */
> +#define SBTSI_CONFIG_EXT_RAGE_SHIFT	2
> +
> +#define SBTSI_TEMP_EXT_RAGE_ADJ	49000

Lower bits first, please. Also, I am quite sure that this should be "RANGE",
not "RAGE".

>   
>   #define SBTSI_TEMP_MIN	0
>   #define SBTSI_TEMP_MAX	255875
> @@ -74,7 +82,12 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
>   {
>   	struct sbtsi_data *data = dev_get_drvdata(dev);
>   	s32 temp_int, temp_dec;
> -	int err;
> +	int err, cfg;
> +
> +	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> +	if (err < 0)
> +		return err;
> +	cfg = err;
>   

Since the configuration byte is now needed in two functions, and since it is
not expected to change dynamically, please store the configuration in struct
sbtsi_data in the probe function and read it from there.

Thanks,
Guenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 0/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-10  8:43 [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support Chuande Chen
  2025-08-10 23:48 ` Guenter Roeck
@ 2025-08-11  5:07 ` Chuande Chen
  2025-08-11  5:07   ` [PATCH v2 1/1] " Chuande Chen
  2025-08-11 14:04   ` [PATCH v2 0/1] " Guenter Roeck
  2025-08-11 17:16 ` [PATCH v3 1/1] " Chuande Chen
  2025-08-14  5:39 ` Chuande Chen
  3 siblings, 2 replies; 9+ messages in thread
From: Chuande Chen @ 2025-08-11  5:07 UTC (permalink / raw)
  To: jdelvare; +Cc: linux, linux-hwmon, linux-kernel, chuachen, chenchuande, groeck7

From: Chuande Chen <chuachen@cisco.com>

V1 -> V2: addressed review comments, also save READ_ORDER bit into sbtsi_data

Chuande Chen (1):
  hwmon: sbtsi_temp: AMD CPU extended temperature range support

 drivers/hwmon/sbtsi_temp.c | 46 +++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 13 deletions(-)

-- 
2.39.5 (Apple Git-154)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-11  5:07 ` [PATCH v2 0/1] " Chuande Chen
@ 2025-08-11  5:07   ` Chuande Chen
  2025-08-11 14:03     ` Guenter Roeck
  2025-08-11 14:04   ` [PATCH v2 0/1] " Guenter Roeck
  1 sibling, 1 reply; 9+ messages in thread
From: Chuande Chen @ 2025-08-11  5:07 UTC (permalink / raw)
  To: jdelvare; +Cc: linux, linux-hwmon, linux-kernel, chuachen, chenchuande, groeck7

From: Chuande Chen <chuachen@cisco.com>

Many AMD CPUs can support this feature now.
We would get a wrong CPU DIE temp if don't consider this.
In low-temperature environments, the CPU die temperature
can drop below zero.
So many platform would like to make extended temperature range
as their default configuration.
Default temperature range (0C to 255.875C) degree celsius.
Extended temperature range (-49C to +206.875C) degree celsius.
Ref Doc: AMD V3000 PPR (Doc ID #56558).

Signed-off-by: Chuande Chen <chuachen@cisco.com>
---
V1 -> V2: addressed review comments, also save READ_ORDER bit into sbtsi_data
---
 drivers/hwmon/sbtsi_temp.c | 46 +++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
index 3c839f56c..32d6a7162 100644
--- a/drivers/hwmon/sbtsi_temp.c
+++ b/drivers/hwmon/sbtsi_temp.c
@@ -5,6 +5,7 @@
  *
  * Copyright (c) 2020, Google Inc.
  * Copyright (c) 2020, Kun Yi <kunyi@google.com>
+ * Copyright (c) 2025, Chuande Chen <chuachen@cisco.com>
  */
 
 #include <linux/err.h>
@@ -14,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/bitfield.h>
 
 /*
  * SB-TSI registers only support SMBus byte data access. "_INT" registers are
@@ -29,8 +31,23 @@
 #define SBTSI_REG_TEMP_HIGH_DEC		0x13 /* RW */
 #define SBTSI_REG_TEMP_LOW_DEC		0x14 /* RW */
 
+/*
+ * Bit for reporting value with temperature measurement range.
+ * bit == 0: Use default temperature range (0C to 255.875C).
+ * bit == 1: Use extended temperature range (-49C to +206.875C).
+ */
+#define SBTSI_CONFIG_EXT_RANGE_SHIFT	2
+/*
+ * ReadOrder bit specifies the reading order of integer and
+ * decimal part of CPU temp for atomic reads. If bit == 0,
+ * reading integer part triggers latching of the decimal part,
+ * so integer part should be read first. If bit == 1, read
+ * order should be reversed.
+ */
 #define SBTSI_CONFIG_READ_ORDER_SHIFT	5
 
+#define SBTSI_TEMP_EXT_RANGE_ADJ	49000
+
 #define SBTSI_TEMP_MIN	0
 #define SBTSI_TEMP_MAX	255875
 
@@ -38,6 +55,8 @@
 struct sbtsi_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	bool ext_range_mode;
+	bool read_order;
 };
 
 /*
@@ -74,23 +93,11 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct sbtsi_data *data = dev_get_drvdata(dev);
 	s32 temp_int, temp_dec;
-	int err;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		/*
-		 * ReadOrder bit specifies the reading order of integer and
-		 * decimal part of CPU temp for atomic reads. If bit == 0,
-		 * reading integer part triggers latching of the decimal part,
-		 * so integer part should be read first. If bit == 1, read
-		 * order should be reversed.
-		 */
-		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
-		if (err < 0)
-			return err;
-
 		mutex_lock(&data->lock);
-		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
+		if (data->read_order) {
 			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
 			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
 		} else {
@@ -122,6 +129,8 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 		return temp_dec;
 
 	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
+	if (data->ext_range_mode)
+		*val -= SBTSI_TEMP_EXT_RANGE_ADJ;
 
 	return 0;
 }
@@ -146,6 +155,8 @@ static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
 		return -EINVAL;
 	}
 
+	if (data->ext_range_mode)
+		val += SBTSI_TEMP_EXT_RANGE_ADJ;
 	val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
 	sbtsi_mc_to_reg(val, &temp_int, &temp_dec);
 
@@ -203,6 +214,7 @@ static int sbtsi_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct sbtsi_data *data;
+	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct sbtsi_data), GFP_KERNEL);
 	if (!data)
@@ -214,6 +226,14 @@ static int sbtsi_probe(struct i2c_client *client)
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &sbtsi_chip_info,
 							 NULL);
 
+	mutex_lock(&data->lock);
+	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
+	mutex_unlock(&data->lock);
+	if (err < 0)
+		return err;
+	data->ext_range_mode = FIELD_GET(BIT(SBTSI_CONFIG_EXT_RANGE_SHIFT), err);
+	data->read_order = FIELD_GET(BIT(SBTSI_CONFIG_READ_ORDER_SHIFT), err);
+
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.39.5 (Apple Git-154)


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-11  5:07   ` [PATCH v2 1/1] " Chuande Chen
@ 2025-08-11 14:03     ` Guenter Roeck
  0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-08-11 14:03 UTC (permalink / raw)
  To: Chuande Chen, jdelvare; +Cc: linux-hwmon, linux-kernel, chuachen, groeck7

On 8/10/25 22:07, Chuande Chen wrote:
> From: Chuande Chen <chuachen@cisco.com>
> 
Any reason for not sending this from your Cisco e-mail address ?

> Many AMD CPUs can support this feature now.
> We would get a wrong CPU DIE temp if don't consider this.

temperature.

> In low-temperature environments, the CPU die temperature
> can drop below zero.
> So many platform would like to make extended temperature range
> as their default configuration.
> Default temperature range (0C to 255.875C) degree celsius.
> Extended temperature range (-49C to +206.875C) degree celsius.

Celsius. But then "C" and "degree celsius" is redundant and "degree celsius"
can be dropped.

> Ref Doc: AMD V3000 PPR (Doc ID #56558).
> 

The document is not available to the public. You will need to find someone
from AMD to provide an Acked-by: or Reviewed-by: tag to show me that the
V3000 hardware really uses this bit.

The column limit for patch descriptions is 75 characters. Please use it.

> Signed-off-by: Chuande Chen <chuachen@cisco.com>
> ---
> V1 -> V2: addressed review comments, also save READ_ORDER bit into sbtsi_data
> ---
>   drivers/hwmon/sbtsi_temp.c | 46 +++++++++++++++++++++++++++-----------
>   1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> index 3c839f56c..32d6a7162 100644
> --- a/drivers/hwmon/sbtsi_temp.c
> +++ b/drivers/hwmon/sbtsi_temp.c
> @@ -5,6 +5,7 @@
>    *
>    * Copyright (c) 2020, Google Inc.
>    * Copyright (c) 2020, Kun Yi <kunyi@google.com>
> + * Copyright (c) 2025, Chuande Chen <chuachen@cisco.com>

For adding a few lines of code ? If everyone would do that, the Linux kernel
code would consist of 50% or more copyright lines. If you insist on this,
I'll have to try and find official guidelines about if and when it is appropriate
to add copyrights.

Plus this made me suspicious: How do I know that you are really a Cisco employee
and not someone who tries to sneak in some code that isn't even supported
by real hardware ?

>    */
>   
>   #include <linux/err.h>
> @@ -14,6 +15,7 @@
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/of.h>
> +#include <linux/bitfield.h>
>   
>   /*
>    * SB-TSI registers only support SMBus byte data access. "_INT" registers are
> @@ -29,8 +31,23 @@
>   #define SBTSI_REG_TEMP_HIGH_DEC		0x13 /* RW */
>   #define SBTSI_REG_TEMP_LOW_DEC		0x14 /* RW */
>   
> +/*
> + * Bit for reporting value with temperature measurement range.
> + * bit == 0: Use default temperature range (0C to 255.875C).
> + * bit == 1: Use extended temperature range (-49C to +206.875C).
> + */
> +#define SBTSI_CONFIG_EXT_RANGE_SHIFT	2
> +/*
> + * ReadOrder bit specifies the reading order of integer and
> + * decimal part of CPU temp for atomic reads. If bit == 0,
> + * reading integer part triggers latching of the decimal part,
> + * so integer part should be read first. If bit == 1, read
> + * order should be reversed.
> + */

Too many line splits. Extend lines to 80 characters. Yes, I see this is copied
from below, but that doesn't mean it can not be adjusted.


>   #define SBTSI_CONFIG_READ_ORDER_SHIFT	5
>   
> +#define SBTSI_TEMP_EXT_RANGE_ADJ	49000
> +
>   #define SBTSI_TEMP_MIN	0
>   #define SBTSI_TEMP_MAX	255875
>   
> @@ -38,6 +55,8 @@
>   struct sbtsi_data {
>   	struct i2c_client *client;
>   	struct mutex lock;
> +	bool ext_range_mode;
> +	bool read_order;
>   };
>   
>   /*
> @@ -74,23 +93,11 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
>   {
>   	struct sbtsi_data *data = dev_get_drvdata(dev);
>   	s32 temp_int, temp_dec;
> -	int err;
>   
>   	switch (attr) {
>   	case hwmon_temp_input:
> -		/*
> -		 * ReadOrder bit specifies the reading order of integer and
> -		 * decimal part of CPU temp for atomic reads. If bit == 0,
> -		 * reading integer part triggers latching of the decimal part,
> -		 * so integer part should be read first. If bit == 1, read
> -		 * order should be reversed.
> -		 */
> -		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> -		if (err < 0)
> -			return err;
> -
>   		mutex_lock(&data->lock);
> -		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> +		if (data->read_order) {
>   			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
>   			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
>   		} else {
> @@ -122,6 +129,8 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
>   		return temp_dec;
>   
>   	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
> +	if (data->ext_range_mode)
> +		*val -= SBTSI_TEMP_EXT_RANGE_ADJ;
>   
>   	return 0;
>   }
> @@ -146,6 +155,8 @@ static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
>   		return -EINVAL;
>   	}
>   
> +	if (data->ext_range_mode)
> +		val += SBTSI_TEMP_EXT_RANGE_ADJ;
>   	val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
>   	sbtsi_mc_to_reg(val, &temp_int, &temp_dec);
>   
> @@ -203,6 +214,7 @@ static int sbtsi_probe(struct i2c_client *client)
>   	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct sbtsi_data *data;
> +	int err;
>   
>   	data = devm_kzalloc(dev, sizeof(struct sbtsi_data), GFP_KERNEL);
>   	if (!data)
> @@ -214,6 +226,14 @@ static int sbtsi_probe(struct i2c_client *client)
>   	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &sbtsi_chip_info,
>   							 NULL);
>   
> +	mutex_lock(&data->lock);
> +	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> +	mutex_unlock(&data->lock);

Unnecessary lock. First, this is in the probe function, where a lock is not needed
in the first place. Second, it is a single I2C operation, which does not need
a lock either case.

On top of that, this needs to be called before the call to
devm_hwmon_device_register_with_info() because otherwise the first read can
happen before the bits are set.

Guenter

> +	if (err < 0)
> +		return err;
> +	data->ext_range_mode = FIELD_GET(BIT(SBTSI_CONFIG_EXT_RANGE_SHIFT), err);
> +	data->read_order = FIELD_GET(BIT(SBTSI_CONFIG_READ_ORDER_SHIFT), err);
> +
>   	return PTR_ERR_OR_ZERO(hwmon_dev);
>   }
>   


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-11  5:07 ` [PATCH v2 0/1] " Chuande Chen
  2025-08-11  5:07   ` [PATCH v2 1/1] " Chuande Chen
@ 2025-08-11 14:04   ` Guenter Roeck
  1 sibling, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-08-11 14:04 UTC (permalink / raw)
  To: Chuande Chen, jdelvare; +Cc: linux-hwmon, linux-kernel, chuachen, groeck7

On 8/10/25 22:07, Chuande Chen wrote:
> From: Chuande Chen <chuachen@cisco.com>
> 
> V1 -> V2: addressed review comments, also save READ_ORDER bit into sbtsi_data
> 
> Chuande Chen (1):
>    hwmon: sbtsi_temp: AMD CPU extended temperature range support
> 
>   drivers/hwmon/sbtsi_temp.c | 46 +++++++++++++++++++++++++++-----------
>   1 file changed, 33 insertions(+), 13 deletions(-)
> 

Introductory patches are not needed for individual patches and only add noise.
Please just send the patch with change log after "---".

Guenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-10  8:43 [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support Chuande Chen
  2025-08-10 23:48 ` Guenter Roeck
  2025-08-11  5:07 ` [PATCH v2 0/1] " Chuande Chen
@ 2025-08-11 17:16 ` Chuande Chen
  2025-08-14  5:39 ` Chuande Chen
  3 siblings, 0 replies; 9+ messages in thread
From: Chuande Chen @ 2025-08-11 17:16 UTC (permalink / raw)
  To: jdelvare; +Cc: linux, linux-hwmon, linux-kernel, chuachen, chenchuande, groeck7

From: Chuande Chen <chuachen@cisco.com>

Many AMD CPUs can support this feature now. We would get a wrong CPU DIE
temperature if don't consider this. In low-temperature environments,
the CPU die temperature can drop below zero. So many platforms would like
to make extended temperature range as their default configuration.
Default temperature range (0C to 255.875C).
Extended temperature range (-49C to +206.875C).
Ref Doc: AMD V3000 PPR (Doc ID #56558).

Signed-off-by: Chuande Chen <chuachen@cisco.com>
---
V2 -> V3: Addressed review comments. Remove my name from Copyright.
          In sbtsi_probe(), removed unnecessary lock, save config before call
          devm_hwmon_device_register_with_info().
V1 -> V2: addressed review comments, also save READ_ORDER bit into sbtsi_data
---
 drivers/hwmon/sbtsi_temp.c | 46 +++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
index 3c839f56c..a6c439e37 100644
--- a/drivers/hwmon/sbtsi_temp.c
+++ b/drivers/hwmon/sbtsi_temp.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/bitfield.h>
 
 /*
  * SB-TSI registers only support SMBus byte data access. "_INT" registers are
@@ -29,8 +30,22 @@
 #define SBTSI_REG_TEMP_HIGH_DEC		0x13 /* RW */
 #define SBTSI_REG_TEMP_LOW_DEC		0x14 /* RW */
 
+/*
+ * Bit for reporting value with temperature measurement range.
+ * bit == 0: Use default temperature range (0C to 255.875C).
+ * bit == 1: Use extended temperature range (-49C to +206.875C).
+ */
+#define SBTSI_CONFIG_EXT_RANGE_SHIFT	2
+/*
+ * ReadOrder bit specifies the reading order of integer and decimal part of
+ * CPU temperature for atomic reads. If bit == 0, reading integer part triggers
+ * latching of the decimal part, so integer part should be read first.
+ * If bit == 1, read order should be reversed.
+ */
 #define SBTSI_CONFIG_READ_ORDER_SHIFT	5
 
+#define SBTSI_TEMP_EXT_RANGE_ADJ	49000
+
 #define SBTSI_TEMP_MIN	0
 #define SBTSI_TEMP_MAX	255875
 
@@ -38,6 +53,8 @@
 struct sbtsi_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	bool ext_range_mode;
+	bool read_order;
 };
 
 /*
@@ -74,23 +91,11 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct sbtsi_data *data = dev_get_drvdata(dev);
 	s32 temp_int, temp_dec;
-	int err;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		/*
-		 * ReadOrder bit specifies the reading order of integer and
-		 * decimal part of CPU temp for atomic reads. If bit == 0,
-		 * reading integer part triggers latching of the decimal part,
-		 * so integer part should be read first. If bit == 1, read
-		 * order should be reversed.
-		 */
-		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
-		if (err < 0)
-			return err;
-
 		mutex_lock(&data->lock);
-		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
+		if (data->read_order) {
 			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
 			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
 		} else {
@@ -122,6 +127,8 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 		return temp_dec;
 
 	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
+	if (data->ext_range_mode)
+		*val -= SBTSI_TEMP_EXT_RANGE_ADJ;
 
 	return 0;
 }
@@ -146,6 +153,8 @@ static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
 		return -EINVAL;
 	}
 
+	if (data->ext_range_mode)
+		val += SBTSI_TEMP_EXT_RANGE_ADJ;
 	val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
 	sbtsi_mc_to_reg(val, &temp_int, &temp_dec);
 
@@ -203,6 +212,7 @@ static int sbtsi_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct sbtsi_data *data;
+	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct sbtsi_data), GFP_KERNEL);
 	if (!data)
@@ -211,8 +221,14 @@ static int sbtsi_probe(struct i2c_client *client)
 	data->client = client;
 	mutex_init(&data->lock);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &sbtsi_chip_info,
-							 NULL);
+	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
+	if (err < 0)
+		return err;
+	data->ext_range_mode = FIELD_GET(BIT(SBTSI_CONFIG_EXT_RANGE_SHIFT), err);
+	data->read_order = FIELD_GET(BIT(SBTSI_CONFIG_READ_ORDER_SHIFT), err);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+							 &sbtsi_chip_info, NULL);
 
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
-- 
2.39.5 (Apple Git-154)


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 1/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-10  8:43 [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support Chuande Chen
                   ` (2 preceding siblings ...)
  2025-08-11 17:16 ` [PATCH v3 1/1] " Chuande Chen
@ 2025-08-14  5:39 ` Chuande Chen
  2025-08-27 16:05   ` Guenter Roeck
  3 siblings, 1 reply; 9+ messages in thread
From: Chuande Chen @ 2025-08-14  5:39 UTC (permalink / raw)
  To: jdelvare
  Cc: linux, linux-hwmon, linux-kernel, chuachen, chenchuande,
	arun.george, groeck7

From: Chuande Chen <chuachen@cisco.com>

Many AMD CPUs can support this feature now. We would get a wrong CPU DIE
temperature if don't consider this. In low-temperature environments,
the CPU die temperature can drop below zero. So many platforms would like
to make extended temperature range as their default configuration.
Default temperature range (0C to 255.875C).
Extended temperature range (-49C to +206.875C).
Ref Doc: AMD V3000 PPR (Doc ID #56558).

Signed-off-by: Chuande Chen <chuachen@cisco.com>
---
V2 -> V3: Addressed review comments. Remove my name from Copyright.
          In sbtsi_probe(), removed unnecessary lock, save config before call
          devm_hwmon_device_register_with_info().
V1 -> V2: addressed review comments, also save READ_ORDER bit into sbtsi_data
---
 drivers/hwmon/sbtsi_temp.c | 46 +++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
index 3c839f56c..a6c439e37 100644
--- a/drivers/hwmon/sbtsi_temp.c
+++ b/drivers/hwmon/sbtsi_temp.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/bitfield.h>
 
 /*
  * SB-TSI registers only support SMBus byte data access. "_INT" registers are
@@ -29,8 +30,22 @@
 #define SBTSI_REG_TEMP_HIGH_DEC		0x13 /* RW */
 #define SBTSI_REG_TEMP_LOW_DEC		0x14 /* RW */
 
+/*
+ * Bit for reporting value with temperature measurement range.
+ * bit == 0: Use default temperature range (0C to 255.875C).
+ * bit == 1: Use extended temperature range (-49C to +206.875C).
+ */
+#define SBTSI_CONFIG_EXT_RANGE_SHIFT	2
+/*
+ * ReadOrder bit specifies the reading order of integer and decimal part of
+ * CPU temperature for atomic reads. If bit == 0, reading integer part triggers
+ * latching of the decimal part, so integer part should be read first.
+ * If bit == 1, read order should be reversed.
+ */
 #define SBTSI_CONFIG_READ_ORDER_SHIFT	5
 
+#define SBTSI_TEMP_EXT_RANGE_ADJ	49000
+
 #define SBTSI_TEMP_MIN	0
 #define SBTSI_TEMP_MAX	255875
 
@@ -38,6 +53,8 @@
 struct sbtsi_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	bool ext_range_mode;
+	bool read_order;
 };
 
 /*
@@ -74,23 +91,11 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct sbtsi_data *data = dev_get_drvdata(dev);
 	s32 temp_int, temp_dec;
-	int err;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		/*
-		 * ReadOrder bit specifies the reading order of integer and
-		 * decimal part of CPU temp for atomic reads. If bit == 0,
-		 * reading integer part triggers latching of the decimal part,
-		 * so integer part should be read first. If bit == 1, read
-		 * order should be reversed.
-		 */
-		err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
-		if (err < 0)
-			return err;
-
 		mutex_lock(&data->lock);
-		if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
+		if (data->read_order) {
 			temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
 			temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
 		} else {
@@ -122,6 +127,8 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
 		return temp_dec;
 
 	*val = sbtsi_reg_to_mc(temp_int, temp_dec);
+	if (data->ext_range_mode)
+		*val -= SBTSI_TEMP_EXT_RANGE_ADJ;
 
 	return 0;
 }
@@ -146,6 +153,8 @@ static int sbtsi_write(struct device *dev, enum hwmon_sensor_types type,
 		return -EINVAL;
 	}
 
+	if (data->ext_range_mode)
+		val += SBTSI_TEMP_EXT_RANGE_ADJ;
 	val = clamp_val(val, SBTSI_TEMP_MIN, SBTSI_TEMP_MAX);
 	sbtsi_mc_to_reg(val, &temp_int, &temp_dec);
 
@@ -203,6 +212,7 @@ static int sbtsi_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct sbtsi_data *data;
+	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct sbtsi_data), GFP_KERNEL);
 	if (!data)
@@ -211,8 +221,14 @@ static int sbtsi_probe(struct i2c_client *client)
 	data->client = client;
 	mutex_init(&data->lock);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &sbtsi_chip_info,
-							 NULL);
+	err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
+	if (err < 0)
+		return err;
+	data->ext_range_mode = FIELD_GET(BIT(SBTSI_CONFIG_EXT_RANGE_SHIFT), err);
+	data->read_order = FIELD_GET(BIT(SBTSI_CONFIG_READ_ORDER_SHIFT), err);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+							 &sbtsi_chip_info, NULL);
 
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
-- 
2.39.5 (Apple Git-154)


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/1] hwmon: sbtsi_temp: AMD CPU extended temperature range support
  2025-08-14  5:39 ` Chuande Chen
@ 2025-08-27 16:05   ` Guenter Roeck
  0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2025-08-27 16:05 UTC (permalink / raw)
  To: Chuande Chen
  Cc: jdelvare, linux-hwmon, linux-kernel, chuachen, arun.george,
	groeck7

On Thu, Aug 14, 2025 at 01:39:40PM +0800, Chuande Chen wrote:
> From: Chuande Chen <chuachen@cisco.com>
> 
> Many AMD CPUs can support this feature now. We would get a wrong CPU DIE
> temperature if don't consider this. In low-temperature environments,
> the CPU die temperature can drop below zero. So many platforms would like
> to make extended temperature range as their default configuration.
> Default temperature range (0C to 255.875C).
> Extended temperature range (-49C to +206.875C).
> Ref Doc: AMD V3000 PPR (Doc ID #56558).
> 
> Signed-off-by: Chuande Chen <chuachen@cisco.com>

Applied.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-08-27 16:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-10  8:43 [PATCH] hwmon: sbtsi_temp: AMD CPU extended temperature range support Chuande Chen
2025-08-10 23:48 ` Guenter Roeck
2025-08-11  5:07 ` [PATCH v2 0/1] " Chuande Chen
2025-08-11  5:07   ` [PATCH v2 1/1] " Chuande Chen
2025-08-11 14:03     ` Guenter Roeck
2025-08-11 14:04   ` [PATCH v2 0/1] " Guenter Roeck
2025-08-11 17:16 ` [PATCH v3 1/1] " Chuande Chen
2025-08-14  5:39 ` Chuande Chen
2025-08-27 16:05   ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).