hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

[PATCH] hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

Posted by Mario Limonciello 1 year, 5 months ago

From: Mario Limonciello <mario.limonciello@amd.com>

This removes some boilerplate from the code and will allow adding
future CPUs by just device IDs.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hwmon/k10temp.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 543526bac0425..85a7632f3b50a 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -438,16 +438,21 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		data->disp_negative = true;
 	}
 
-	if (boot_cpu_data.x86 == 0x15 &&
+	data->is_zen = cpu_feature_enabled(X86_FEATURE_ZEN);
+	if (data->is_zen) {
+		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
+		data->read_tempreg = read_tempreg_nb_zen;
+	} else if (boot_cpu_data.x86 == 0x15 &&
 	    ((boot_cpu_data.x86_model & 0xf0) == 0x60 ||
 	     (boot_cpu_data.x86_model & 0xf0) == 0x70)) {
 		data->read_htcreg = read_htcreg_nb_f15;
 		data->read_tempreg = read_tempreg_nb_f15;
-	} else if (boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 0x18) {
-		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
-		data->read_tempreg = read_tempreg_nb_zen;
-		data->is_zen = true;
+	} else {
+		data->read_htcreg = read_htcreg_pci;
+		data->read_tempreg = read_tempreg_pci;
+	}
 
+	if (boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 0x18) {
 		switch (boot_cpu_data.x86_model) {
 		case 0x1:	/* Zen */
 		case 0x8:	/* Zen+ */
@@ -469,10 +474,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 			break;
 		}
 	} else if (boot_cpu_data.x86 == 0x19) {
-		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
-		data->read_tempreg = read_tempreg_nb_zen;
-		data->is_zen = true;
-
 		switch (boot_cpu_data.x86_model) {
 		case 0x0 ... 0x1:	/* Zen3 SP3/TR */
 		case 0x8:		/* Zen3 TR Chagall */
@@ -496,13 +497,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 			k10temp_get_ccd_support(data, 12);
 			break;
 		}
-	} else if (boot_cpu_data.x86 == 0x1a) {
-		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
-		data->read_tempreg = read_tempreg_nb_zen;
-		data->is_zen = true;
-	} else {
-		data->read_htcreg = read_htcreg_pci;
-		data->read_tempreg = read_tempreg_pci;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(tctl_offset_table); i++) {
-- 
2.43.0

Re: [PATCH] hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

Posted by Guenter Roeck 1 year, 5 months ago

On Tue, Aug 20, 2024 at 12:35:57AM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> This removes some boilerplate from the code and will allow adding
> future CPUs by just device IDs.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Applied.

Thanks,
Guenter

Re: [PATCH] hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

Posted by Yazen Ghannam 1 year, 5 months ago

On Tue, Aug 20, 2024 at 12:35:57AM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> This removes some boilerplate from the code and will allow adding
> future CPUs by just device IDs.
>

I've been thinking that maybe we stop using PCI IDs entirely.

The PCIe devices that we match on are internal to the SoC. And they're
not optional. They're basically processor components that are exposed
through PCI config space for software convenience.

Thoughts?

Thanks,
Yazen

Re: [PATCH] hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

Posted by Mario Limonciello 1 year, 5 months ago

On 8/21/2024 08:09, Yazen Ghannam wrote:
> On Tue, Aug 20, 2024 at 12:35:57AM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> This removes some boilerplate from the code and will allow adding
>> future CPUs by just device IDs.
>>
> 
> I've been thinking that maybe we stop using PCI IDs entirely.
> 
> The PCIe devices that we match on are internal to the SoC. And they're
> not optional. They're basically processor components that are exposed
> through PCI config space for software convenience.
> 
> Thoughts?
> 
> Thanks,
> Yazen

Yeah I think that's a good idea.  The one thing I want to make sure 
remains though is that k10temp automatically loads from a CPU modalias.

This is "tangential" to this patch except for the commit message 
reference to the PCI IDs.

Re: [PATCH] hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

Posted by Guenter Roeck 1 year, 5 months ago

On Wed, Aug 21, 2024 at 09:34:09AM -0500, Mario Limonciello wrote:
> On 8/21/2024 08:09, Yazen Ghannam wrote:
> > On Tue, Aug 20, 2024 at 12:35:57AM -0500, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > This removes some boilerplate from the code and will allow adding
> > > future CPUs by just device IDs.
> > > 
> > 
> > I've been thinking that maybe we stop using PCI IDs entirely.
> > 
> > The PCIe devices that we match on are internal to the SoC. And they're
> > not optional. They're basically processor components that are exposed
> > through PCI config space for software convenience.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Yazen
> 
> Yeah I think that's a good idea.  The one thing I want to make sure remains
> though is that k10temp automatically loads from a CPU modalias.
> 
> This is "tangential" to this patch except for the commit message reference
> to the PCI IDs.
> 

Based on the feedback I will not accept this patch but wait for a solution
which is acceptable for everyone involved from AMD.

Guenter

Re: [PATCH] hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen

Posted by Yazen Ghannam 1 year, 5 months ago

On Wed, Aug 21, 2024 at 04:29:32PM -0700, Guenter Roeck wrote:
> On Wed, Aug 21, 2024 at 09:34:09AM -0500, Mario Limonciello wrote:
> > On 8/21/2024 08:09, Yazen Ghannam wrote:
> > > On Tue, Aug 20, 2024 at 12:35:57AM -0500, Mario Limonciello wrote:
> > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > 
> > > > This removes some boilerplate from the code and will allow adding
> > > > future CPUs by just device IDs.
> > > > 
> > > 
> > > I've been thinking that maybe we stop using PCI IDs entirely.
> > > 
> > > The PCIe devices that we match on are internal to the SoC. And they're
> > > not optional. They're basically processor components that are exposed
> > > through PCI config space for software convenience.
> > > 
> > > Thoughts?
> > > 
> > > Thanks,
> > > Yazen
> > 
> > Yeah I think that's a good idea.  The one thing I want to make sure remains
> > though is that k10temp automatically loads from a CPU modalias.
> > 
> > This is "tangential" to this patch except for the commit message reference
> > to the PCI IDs.
> > 
> 
> Based on the feedback I will not accept this patch but wait for a solution
> which is acceptable for everyone involved from AMD.
> 

Hi Guenter,

Sorry, I didn't mean to imply that this is unacceptable. I think this is
okay. I just wanted to highlight our general trend to move away from
continuing to add PCI IDs. We'll still need to do so in the short term
though.

Mario and I are working on some possible solutions. It'll be slow and
iterative, since we don't want to break existing functionality.

Thanks,
Yazen