It seems appropriate to move the CCD specific information inside the
k10temp_data structure.
Signed-off-by: Babu Moger <[email protected]>
---
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), ®val);
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 {
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 <[email protected]>
---
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),
®val);
@@ -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;
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 <[email protected]>
> ---
> 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), ®val);
> 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 {
>
>
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 <[email protected]>
>> ---
>> 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), ®val);
>> 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 {
>>
>>
>
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
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