diff mbox series

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

Message ID 20240820053558.1052853-1-superm1@kernel.org (mailing list archive)
State Accepted
Headers show
Series hwmon: (k10temp): Use cpu_feature_enabled() for detecting zen | expand

Commit Message

Mario Limonciello Aug. 20, 2024, 5:35 a.m. UTC 
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(-)

Comments

Yazen Ghannam Aug. 21, 2024, 1:09 p.m. UTC | #1 
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
Mario Limonciello Aug. 21, 2024, 2:34 p.m. UTC | #2 
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.
Guenter Roeck Aug. 21, 2024, 11:29 p.m. UTC | #3 
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
Yazen Ghannam Aug. 22, 2024, 2:19 p.m. UTC | #4 
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
Guenter Roeck Aug. 27, 2024, 3:57 a.m. UTC | #5 
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
diff mbox series

Patch

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++) {