linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure
@ 2021-11-23 21:16 Babu Moger
  2021-11-23 21:16 ` [PATCH 2/2] hwmon: (k10temp) Support up to 12 CCDs on AMD Family of processors Babu Moger
  2021-11-23 21:40 ` [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Guenter Roeck
  0 siblings, 2 replies; 6+ messages in thread
From: Babu Moger @ 2021-11-23 21:16 UTC (permalink / raw)
  To: clemens, jdelvare, linux; +Cc: linux-hwmon, linux-kernel

It seems appropriate to move the CCD specific information inside the
k10temp_data structure.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
Note: Generated the patch on top of hwmon-next.

 drivers/hwmon/k10temp.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 880990fa4795..bd436b380a02 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -85,6 +85,7 @@ struct k10temp_data {
 	u32 show_temp;
 	bool is_zen;
 	u32 ccd_offset;
+	u32 ccd_limit;
 };
 
 #define TCTL_BIT	0
@@ -357,12 +358,12 @@ static const struct hwmon_chip_info k10temp_chip_info = {
 };
 
 static void k10temp_get_ccd_support(struct pci_dev *pdev,
-				    struct k10temp_data *data, int limit)
+				    struct k10temp_data *data)
 {
 	u32 regval;
 	int i;
 
-	for (i = 0; i < limit; i++) {
+	for (i = 0; i < data->ccd_limit; i++) {
 		amd_smn_read(amd_pci_dev_to_node_id(pdev),
 			     ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
 		if (regval & ZEN_CCD_TEMP_VALID)
@@ -411,14 +412,16 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		case 0x11:	/* Zen APU */
 		case 0x18:	/* Zen+ APU */
 			data->ccd_offset = 0x154;
-			k10temp_get_ccd_support(pdev, data, 4);
+			data->ccd_limit = 4;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		case 0x31:	/* Zen2 Threadripper */
 		case 0x60:	/* Renoir */
 		case 0x68:	/* Lucienne */
 		case 0x71:	/* Zen2 */
 			data->ccd_offset = 0x154;
-			k10temp_get_ccd_support(pdev, data, 8);
+			data->ccd_limit = 8;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		}
 	} else if (boot_cpu_data.x86 == 0x19) {
@@ -431,13 +434,15 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		case 0x21:		/* Zen3 Ryzen Desktop */
 		case 0x50 ... 0x5f:	/* Green Sardine */
 			data->ccd_offset = 0x154;
-			k10temp_get_ccd_support(pdev, data, 8);
+			data->ccd_limit = 8;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		case 0x10 ... 0x1f:
 		case 0x40 ... 0x4f:	/* Yellow Carp */
 		case 0xa0 ... 0xaf:
 			data->ccd_offset = 0x300;
-			k10temp_get_ccd_support(pdev, data, 8);
+			data->ccd_limit = 8;
+			k10temp_get_ccd_support(pdev, data);
 			break;
 		}
 	} else {



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

* [PATCH 2/2] hwmon: (k10temp) Support up to 12 CCDs on AMD Family of processors
  2021-11-23 21:16 [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Babu Moger
@ 2021-11-23 21:16 ` Babu Moger
  2021-11-23 21:40 ` [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Guenter Roeck
  1 sibling, 0 replies; 6+ messages in thread
From: Babu Moger @ 2021-11-23 21:16 UTC (permalink / raw)
  To: clemens, jdelvare, linux; +Cc: linux-hwmon, linux-kernel

The current driver can read the temperatures from upto 8 CCDs
(Core-Complex Die).

The newer AMD Family 19h Models 10h-1Fh and A0h-AFh can support up to
12 CCDs. Update the driver to read up to 12 CCDs.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
Note: Generated the patch on top of hwmon-next

 drivers/hwmon/k10temp.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index bd436b380a02..d20159e082ab 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -172,6 +172,10 @@ static const char *k10temp_temp_label[] = {
 	"Tccd6",
 	"Tccd7",
 	"Tccd8",
+	"Tccd9",
+	"Tccd10",
+	"Tccd11",
+	"Tccd12",
 };
 
 static int k10temp_read_labels(struct device *dev,
@@ -207,7 +211,7 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
 			if (*val < 0)
 				*val = 0;
 			break;
-		case 2 ... 9:		/* Tccd{1-8} */
+		case 2 ... 13:		/* Tccd{1-12} */
 			amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
 				     ZEN_CCD_TEMP(data->ccd_offset, channel - 2),
 						  &regval);
@@ -342,6 +346,10 @@ static const struct hwmon_channel_info *k10temp_info[] = {
 			   HWMON_T_INPUT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_LABEL),
 	NULL
 };
@@ -437,13 +445,17 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 			data->ccd_limit = 8;
 			k10temp_get_ccd_support(pdev, data);
 			break;
-		case 0x10 ... 0x1f:
 		case 0x40 ... 0x4f:	/* Yellow Carp */
-		case 0xa0 ... 0xaf:
 			data->ccd_offset = 0x300;
 			data->ccd_limit = 8;
 			k10temp_get_ccd_support(pdev, data);
 			break;
+		case 0x10 ... 0x1f:
+		case 0xa0 ... 0xaf:
+			data->ccd_offset = 0x300;
+			data->ccd_limit = 12;
+			k10temp_get_ccd_support(pdev, data);
+			break;
 		}
 	} else {
 		data->read_htcreg = read_htcreg_pci;



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

* Re: [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure
  2021-11-23 21:16 [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Babu Moger
  2021-11-23 21:16 ` [PATCH 2/2] hwmon: (k10temp) Support up to 12 CCDs on AMD Family of processors Babu Moger
@ 2021-11-23 21:40 ` Guenter Roeck
  2021-11-23 21:52   ` Moger, Babu
  1 sibling, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2021-11-23 21:40 UTC (permalink / raw)
  To: Babu Moger, clemens, jdelvare; +Cc: linux-hwmon, linux-kernel

On 11/23/21 1:16 PM, Babu Moger wrote:
> It seems appropriate to move the CCD specific information inside the
> k10temp_data structure.
> 

Why ? I don't see it used outside k10temp_get_ccd_support().

Guenter

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> Note: Generated the patch on top of hwmon-next.
> 
>   drivers/hwmon/k10temp.c |   17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index 880990fa4795..bd436b380a02 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -85,6 +85,7 @@ struct k10temp_data {
>   	u32 show_temp;
>   	bool is_zen;
>   	u32 ccd_offset;
> +	u32 ccd_limit;
>   };
>   
>   #define TCTL_BIT	0
> @@ -357,12 +358,12 @@ static const struct hwmon_chip_info k10temp_chip_info = {
>   };
>   
>   static void k10temp_get_ccd_support(struct pci_dev *pdev,
> -				    struct k10temp_data *data, int limit)
> +				    struct k10temp_data *data)
>   {
>   	u32 regval;
>   	int i;
>   
> -	for (i = 0; i < limit; i++) {
> +	for (i = 0; i < data->ccd_limit; i++) {
>   		amd_smn_read(amd_pci_dev_to_node_id(pdev),
>   			     ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
>   		if (regval & ZEN_CCD_TEMP_VALID)
> @@ -411,14 +412,16 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		case 0x11:	/* Zen APU */
>   		case 0x18:	/* Zen+ APU */
>   			data->ccd_offset = 0x154;
> -			k10temp_get_ccd_support(pdev, data, 4);
> +			data->ccd_limit = 4;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		case 0x31:	/* Zen2 Threadripper */
>   		case 0x60:	/* Renoir */
>   		case 0x68:	/* Lucienne */
>   		case 0x71:	/* Zen2 */
>   			data->ccd_offset = 0x154;
> -			k10temp_get_ccd_support(pdev, data, 8);
> +			data->ccd_limit = 8;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		}
>   	} else if (boot_cpu_data.x86 == 0x19) {
> @@ -431,13 +434,15 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		case 0x21:		/* Zen3 Ryzen Desktop */
>   		case 0x50 ... 0x5f:	/* Green Sardine */
>   			data->ccd_offset = 0x154;
> -			k10temp_get_ccd_support(pdev, data, 8);
> +			data->ccd_limit = 8;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		case 0x10 ... 0x1f:
>   		case 0x40 ... 0x4f:	/* Yellow Carp */
>   		case 0xa0 ... 0xaf:
>   			data->ccd_offset = 0x300;
> -			k10temp_get_ccd_support(pdev, data, 8);
> +			data->ccd_limit = 8;
> +			k10temp_get_ccd_support(pdev, data);
>   			break;
>   		}
>   	} else {
> 
> 


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

* Re: [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure
  2021-11-23 21:40 ` [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Guenter Roeck
@ 2021-11-23 21:52   ` Moger, Babu
  2021-11-23 22:07     ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Moger, Babu @ 2021-11-23 21:52 UTC (permalink / raw)
  To: Guenter Roeck, Babu Moger, clemens, jdelvare; +Cc: linux-hwmon, linux-kernel



On 11/23/2021 3:40 PM, Guenter Roeck wrote:
> On 11/23/21 1:16 PM, Babu Moger wrote:
>> It seems appropriate to move the CCD specific information inside the
>> k10temp_data structure.
>>
> 
> Why ? I don't see it used outside k10temp_get_ccd_support().

Thought it will be cleaner to have it all together at one structure. If
you feel otherwise I can remove it.
Thanks
Babu

> 
> Guenter
> 
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> Note: Generated the patch on top of hwmon-next.
>>
>>   drivers/hwmon/k10temp.c |   17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
>> index 880990fa4795..bd436b380a02 100644
>> --- a/drivers/hwmon/k10temp.c
>> +++ b/drivers/hwmon/k10temp.c
>> @@ -85,6 +85,7 @@ struct k10temp_data {
>>       u32 show_temp;
>>       bool is_zen;
>>       u32 ccd_offset;
>> +    u32 ccd_limit;
>>   };
>>   #define TCTL_BIT    0
>> @@ -357,12 +358,12 @@ static const struct hwmon_chip_info 
>> k10temp_chip_info = {
>>   };
>>   static void k10temp_get_ccd_support(struct pci_dev *pdev,
>> -                    struct k10temp_data *data, int limit)
>> +                    struct k10temp_data *data)
>>   {
>>       u32 regval;
>>       int i;
>> -    for (i = 0; i < limit; i++) {
>> +    for (i = 0; i < data->ccd_limit; i++) {
>>           amd_smn_read(amd_pci_dev_to_node_id(pdev),
>>                    ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
>>           if (regval & ZEN_CCD_TEMP_VALID)
>> @@ -411,14 +412,16 @@ static int k10temp_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>           case 0x11:    /* Zen APU */
>>           case 0x18:    /* Zen+ APU */
>>               data->ccd_offset = 0x154;
>> -            k10temp_get_ccd_support(pdev, data, 4);
>> +            data->ccd_limit = 4;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           case 0x31:    /* Zen2 Threadripper */
>>           case 0x60:    /* Renoir */
>>           case 0x68:    /* Lucienne */
>>           case 0x71:    /* Zen2 */
>>               data->ccd_offset = 0x154;
>> -            k10temp_get_ccd_support(pdev, data, 8);
>> +            data->ccd_limit = 8;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           }
>>       } else if (boot_cpu_data.x86 == 0x19) {
>> @@ -431,13 +434,15 @@ static int k10temp_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>           case 0x21:        /* Zen3 Ryzen Desktop */
>>           case 0x50 ... 0x5f:    /* Green Sardine */
>>               data->ccd_offset = 0x154;
>> -            k10temp_get_ccd_support(pdev, data, 8);
>> +            data->ccd_limit = 8;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           case 0x10 ... 0x1f:
>>           case 0x40 ... 0x4f:    /* Yellow Carp */
>>           case 0xa0 ... 0xaf:
>>               data->ccd_offset = 0x300;
>> -            k10temp_get_ccd_support(pdev, data, 8);
>> +            data->ccd_limit = 8;
>> +            k10temp_get_ccd_support(pdev, data);
>>               break;
>>           }
>>       } else {
>>
>>
> 

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

* Re: [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure
  2021-11-23 21:52   ` Moger, Babu
@ 2021-11-23 22:07     ` Guenter Roeck
  2021-11-23 22:15       ` Babu Moger
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2021-11-23 22:07 UTC (permalink / raw)
  To: babu.moger, clemens, jdelvare; +Cc: linux-hwmon, linux-kernel

On 11/23/21 1:52 PM, Moger, Babu wrote:
> 
> 
> On 11/23/2021 3:40 PM, Guenter Roeck wrote:
>> On 11/23/21 1:16 PM, Babu Moger wrote:
>>> It seems appropriate to move the CCD specific information inside the
>>> k10temp_data structure.
>>>
>>
>> Why ? I don't see it used outside k10temp_get_ccd_support().
> 
> Thought it will be cleaner to have it all together at one structure. If
> you feel otherwise I can remove it.

I don't see the point of having a value in a data structure that isn't
used anywhere outside probing but nevertheless lives forever.

I also don't see how it is "cleaner" to assign a value to a variable
in a data structure only to dereference it in a single called function,
when it can be just as easily be passed as argument to that function.
I would call that just the opposite, since it creates the impression
that the variable is needed outside probing when that is not the case.

Guenter

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

* Re: [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure
  2021-11-23 22:07     ` Guenter Roeck
@ 2021-11-23 22:15       ` Babu Moger
  0 siblings, 0 replies; 6+ messages in thread
From: Babu Moger @ 2021-11-23 22:15 UTC (permalink / raw)
  To: Guenter Roeck, clemens, jdelvare; +Cc: linux-hwmon, linux-kernel


On 11/23/21 4:07 PM, Guenter Roeck wrote:
> On 11/23/21 1:52 PM, Moger, Babu wrote:
>>
>>
>> On 11/23/2021 3:40 PM, Guenter Roeck wrote:
>>> On 11/23/21 1:16 PM, Babu Moger wrote:
>>>> It seems appropriate to move the CCD specific information inside the
>>>> k10temp_data structure.
>>>>
>>>
>>> Why ? I don't see it used outside k10temp_get_ccd_support().
>>
>> Thought it will be cleaner to have it all together at one structure. If
>> you feel otherwise I can remove it.
>
> I don't see the point of having a value in a data structure that isn't
> used anywhere outside probing but nevertheless lives forever.
>
> I also don't see how it is "cleaner" to assign a value to a variable
> in a data structure only to dereference it in a single called function,
> when it can be just as easily be passed as argument to that function.
> I would call that just the opposite, since it creates the impression
> that the variable is needed outside probing when that is not the case.
>  
Ok. Sure. I will remove this change and re-submitt.

-- 
Thanks
Babu Moger


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

end of thread, other threads:[~2021-11-23 22:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 21:16 [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Babu Moger
2021-11-23 21:16 ` [PATCH 2/2] hwmon: (k10temp) Support up to 12 CCDs on AMD Family of processors Babu Moger
2021-11-23 21:40 ` [PATCH 1/2] hwmon: (k10temp) Move the CCD limit info inside k10temp_data structure Guenter Roeck
2021-11-23 21:52   ` Moger, Babu
2021-11-23 22:07     ` Guenter Roeck
2021-11-23 22:15       ` Babu Moger

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).