* [PATCH] hwmon, fam15h_power: Add support for two more processors
@ 2014-09-10 17:02 Aravind Gopalakrishnan
2014-09-10 17:53 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-10 17:02 UTC (permalink / raw)
To: herrmann.der.user, linux, jdelvare, lm-sensors, linux-kernel
Cc: Aravind Gopalakrishnan
Fam16h,M30h(Mullins) and Fam15hM30h(Kaveri) processors can
report 'power_crit' value. So, adding their respective device ids.
Also, according to BKDGs, the 'TdpRunAvgAccCap' that show_power()
uses is valid only on Fam15h, Models 0x0-0xF. On all other processors
the field is 'Reserved'. So, return error if we are on any other family/model.
Impact on lm-sensors is minimal. On such families, instead of reporting
Current power value as '0', we now have:
power1: N/A
Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
drivers/hwmon/fam15h_power.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
index 4a7cbfa..b69bf7d 100644
--- a/drivers/hwmon/fam15h_power.c
+++ b/drivers/hwmon/fam15h_power.c
@@ -57,6 +57,10 @@ static ssize_t show_power(struct device *dev,
struct fam15h_power_data *data = dev_get_drvdata(dev);
struct pci_dev *f4 = data->pdev;
+ /* The value TdpRunAvgAccCap is valid only on F15h, Models 0x0-0xF */
+ if (boot_cpu_data.x86 != 0x15 || boot_cpu_data.x86_model > 0x0)
+ return -ENOSYS;
+
pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
REG_TDP_RUNNING_AVERAGE, &val);
running_avg_capture = (val >> 4) & 0x3fffff;
@@ -216,7 +220,9 @@ static int fam15h_power_probe(struct pci_dev *pdev,
static const struct pci_device_id fam15h_power_id_table[] = {
{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F3) },
{}
};
MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
--
2.0.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon, fam15h_power: Add support for two more processors
2014-09-10 17:02 [PATCH] hwmon, fam15h_power: Add support for two more processors Aravind Gopalakrishnan
@ 2014-09-10 17:53 ` Guenter Roeck
2014-09-10 20:01 ` Aravind Gopalakrishnan
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2014-09-10 17:53 UTC (permalink / raw)
To: Aravind Gopalakrishnan
Cc: herrmann.der.user, jdelvare, lm-sensors, linux-kernel
On Wed, Sep 10, 2014 at 12:02:08PM -0500, Aravind Gopalakrishnan wrote:
> Fam16h,M30h(Mullins) and Fam15hM30h(Kaveri) processors can
> report 'power_crit' value. So, adding their respective device ids.
>
> Also, according to BKDGs, the 'TdpRunAvgAccCap' that show_power()
> uses is valid only on Fam15h, Models 0x0-0xF. On all other processors
> the field is 'Reserved'. So, return error if we are on any other family/model.
>
> Impact on lm-sensors is minimal. On such families, instead of reporting
> Current power value as '0', we now have:
> power1: N/A
>
It will result in people complaining to us about it.
It would be more appropriate to not create the attribute the first place
if it is not supported. Sure, that is a bit more code, but it isn't that bad.
You can simply return -ENODEV for unsupported CPUs from the probe function.
Additional comment below.
Guenter
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
> drivers/hwmon/fam15h_power.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> index 4a7cbfa..b69bf7d 100644
> --- a/drivers/hwmon/fam15h_power.c
> +++ b/drivers/hwmon/fam15h_power.c
> @@ -57,6 +57,10 @@ static ssize_t show_power(struct device *dev,
> struct fam15h_power_data *data = dev_get_drvdata(dev);
> struct pci_dev *f4 = data->pdev;
>
> + /* The value TdpRunAvgAccCap is valid only on F15h, Models 0x0-0xF */
> + if (boot_cpu_data.x86 != 0x15 || boot_cpu_data.x86_model > 0x0)
The comment does not match the code. The comment talks about accepting models
F15h, models 0x0-0xF, but the code rejects anything but F15h model 0x0.
Now it may well be that the above describes identifies all F15h and F16h CPUs,
but this is not clear from the comment. It rather looks as if anything but F15h,
model 0x0 is rejected, including all F16h CPUs. But then why accept F16h CPUs
in the first place ?
> + return -ENOSYS;
> +
> pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
> REG_TDP_RUNNING_AVERAGE, &val);
> running_avg_capture = (val >> 4) & 0x3fffff;
> @@ -216,7 +220,9 @@ static int fam15h_power_probe(struct pci_dev *pdev,
>
> static const struct pci_device_id fam15h_power_id_table[] = {
> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F3) },
> {}
> };
> MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
> --
> 2.0.3
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon, fam15h_power: Add support for two more processors
2014-09-10 17:53 ` Guenter Roeck
@ 2014-09-10 20:01 ` Aravind Gopalakrishnan
2014-09-10 20:37 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-10 20:01 UTC (permalink / raw)
To: Guenter Roeck; +Cc: herrmann.der.user, jdelvare, lm-sensors, linux-kernel
On 9/10/2014 12:53 PM, Guenter Roeck wrote:
> On Wed, Sep 10, 2014 at 12:02:08PM -0500, Aravind Gopalakrishnan wrote:
>> Fam16h,M30h(Mullins) and Fam15hM30h(Kaveri) processors can
>> report 'power_crit' value. So, adding their respective device ids.
>>
>> Also, according to BKDGs, the 'TdpRunAvgAccCap' that show_power()
>> uses is valid only on Fam15h, Models 0x0-0xF. On all other processors
>> the field is 'Reserved'. So, return error if we are on any other family/model.
>>
>> Impact on lm-sensors is minimal. On such families, instead of reporting
>> Current power value as '0', we now have:
>> power1: N/A
>>
> It will result in people complaining to us about it.
>
> It would be more appropriate to not create the attribute the first place
> if it is not supported. Sure, that is a bit more code, but it isn't that bad.
> You can simply return -ENODEV for unsupported CPUs from the probe function.
>
>
>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>> ---
>> drivers/hwmon/fam15h_power.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
>> index 4a7cbfa..b69bf7d 100644
>> --- a/drivers/hwmon/fam15h_power.c
>> +++ b/drivers/hwmon/fam15h_power.c
>> @@ -57,6 +57,10 @@ static ssize_t show_power(struct device *dev,
>> struct fam15h_power_data *data = dev_get_drvdata(dev);
>> struct pci_dev *f4 = data->pdev;
>>
>> + /* The value TdpRunAvgAccCap is valid only on F15h, Models 0x0-0xF */
>> + if (boot_cpu_data.x86 != 0x15 || boot_cpu_data.x86_model > 0x0)
> The comment does not match the code. The comment talks about accepting models
> F15h, models 0x0-0xF, but the code rejects anything but F15h model 0x0.
Ah. Yes, The condition should have been (..boot_cpu_data.x86_model > 0xf)
> Now it may well be that the above describes identifies all F15h and F16h CPUs,
> but this is not clear from the comment. It rather looks as if anything but F15h,
> model 0x0 is rejected, including all F16h CPUs. But then why accept F16h CPUs
> in the first place ?
Yes, we want to reject anything but F15h, Models 00h-0fh.
The reason I included the newer processor IDs, (and let
PCI_DEVICE_ID_AMD_16H_NB_F4) remain
is because we can still obtain 'critical power value'. It is only the
'current power' that is not exposed.
If we return -ENODEV in the probe function (or we can just remove the
listed PCI_DEVICE_ID), then we'd not get the critical power values too.
- Aravind.
>> + return -ENOSYS;
>> +
>> pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
>> REG_TDP_RUNNING_AVERAGE, &val);
>> running_avg_capture = (val >> 4) & 0x3fffff;
>> @@ -216,7 +220,9 @@ static int fam15h_power_probe(struct pci_dev *pdev,
>>
>> static const struct pci_device_id fam15h_power_id_table[] = {
>> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
>> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F3) },
>> {}
>> };
>> MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
>> --
>> 2.0.3
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon, fam15h_power: Add support for two more processors
2014-09-10 20:01 ` Aravind Gopalakrishnan
@ 2014-09-10 20:37 ` Guenter Roeck
2014-09-10 21:23 ` Aravind Gopalakrishnan
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2014-09-10 20:37 UTC (permalink / raw)
To: Aravind Gopalakrishnan
Cc: herrmann.der.user, jdelvare, lm-sensors, linux-kernel,
Boris Ostrovsky
On Wed, Sep 10, 2014 at 03:01:36PM -0500, Aravind Gopalakrishnan wrote:
> On 9/10/2014 12:53 PM, Guenter Roeck wrote:
> >On Wed, Sep 10, 2014 at 12:02:08PM -0500, Aravind Gopalakrishnan wrote:
> >>Fam16h,M30h(Mullins) and Fam15hM30h(Kaveri) processors can
> >>report 'power_crit' value. So, adding their respective device ids.
> >>
> >>Also, according to BKDGs, the 'TdpRunAvgAccCap' that show_power()
> >>uses is valid only on Fam15h, Models 0x0-0xF. On all other processors
> >>the field is 'Reserved'. So, return error if we are on any other family/model.
> >>
> >>Impact on lm-sensors is minimal. On such families, instead of reporting
> >>Current power value as '0', we now have:
> >>power1: N/A
> >>
> >It will result in people complaining to us about it.
> >
> >It would be more appropriate to not create the attribute the first place
> >if it is not supported. Sure, that is a bit more code, but it isn't that bad.
> >You can simply return -ENODEV for unsupported CPUs from the probe function.
> >
> >
> >>Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> >>---
> >> drivers/hwmon/fam15h_power.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >>diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> >>index 4a7cbfa..b69bf7d 100644
> >>--- a/drivers/hwmon/fam15h_power.c
> >>+++ b/drivers/hwmon/fam15h_power.c
> >>@@ -57,6 +57,10 @@ static ssize_t show_power(struct device *dev,
> >> struct fam15h_power_data *data = dev_get_drvdata(dev);
> >> struct pci_dev *f4 = data->pdev;
> >>+ /* The value TdpRunAvgAccCap is valid only on F15h, Models 0x0-0xF */
> >>+ if (boot_cpu_data.x86 != 0x15 || boot_cpu_data.x86_model > 0x0)
> >The comment does not match the code. The comment talks about accepting models
> >F15h, models 0x0-0xF, but the code rejects anything but F15h model 0x0.
>
> Ah. Yes, The condition should have been (..boot_cpu_data.x86_model > 0xf)
>
> >Now it may well be that the above describes identifies all F15h and F16h CPUs,
> >but this is not clear from the comment. It rather looks as if anything but F15h,
> >model 0x0 is rejected, including all F16h CPUs. But then why accept F16h CPUs
> >in the first place ?
>
> Yes, we want to reject anything but F15h, Models 00h-0fh.
> The reason I included the newer processor IDs, (and let
> PCI_DEVICE_ID_AMD_16H_NB_F4) remain
> is because we can still obtain 'critical power value'. It is only
> the 'current power' that is not exposed.
>
That is a behavioral change, though; previously the current power was
reported for F16h chips with PCI ID PCI_DEVICE_ID_AMD_16H_NB_F4.
Is this a bug, ie should the power value not have been reported
for the F16h chips ?
> If we return -ENODEV in the probe function (or we can just remove
> the listed PCI_DEVICE_ID), then we'd not get the critical power
> values too.
>
If you want to make the actual power reporting conditional, you should
introduce an is_visible function to the attribute group to ensure that
power1_input is only reported if/when supported. If the actual power
value is not really supported for F16h chips, you should actually provide
two separate patches: One to make power1_input optional, to be reported for
supported chips only, and another to add more chips. One is a bug fix,
the other a functionality extension.
Guenter
> - Aravind.
>
> >>+ return -ENOSYS;
> >>+
> >> pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
> >> REG_TDP_RUNNING_AVERAGE, &val);
> >> running_avg_capture = (val >> 4) & 0x3fffff;
> >>@@ -216,7 +220,9 @@ static int fam15h_power_probe(struct pci_dev *pdev,
> >> static const struct pci_device_id fam15h_power_id_table[] = {
> >> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> >>+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
> >> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
> >>+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F3) },
> >> {}
> >> };
> >> MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
> >>--
> >>2.0.3
> >>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon, fam15h_power: Add support for two more processors
2014-09-10 20:37 ` Guenter Roeck
@ 2014-09-10 21:23 ` Aravind Gopalakrishnan
0 siblings, 0 replies; 5+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-10 21:23 UTC (permalink / raw)
To: Guenter Roeck
Cc: herrmann.der.user, jdelvare, lm-sensors, linux-kernel,
Boris Ostrovsky
On 9/10/2014 3:37 PM, Guenter Roeck wrote:
> On Wed, Sep 10, 2014 at 03:01:36PM -0500, Aravind Gopalakrishnan wrote:
>> On 9/10/2014 12:53 PM, Guenter Roeck wrote:
>>> On Wed, Sep 10, 2014 at 12:02:08PM -0500, Aravind Gopalakrishnan wrote:
>>>> Fam16h,M30h(Mullins) and Fam15hM30h(Kaveri) processors can
>>>> report 'power_crit' value. So, adding their respective device ids.
>>>>
>>>> Also, according to BKDGs, the 'TdpRunAvgAccCap' that show_power()
>>>> uses is valid only on Fam15h, Models 0x0-0xF. On all other processors
>>>> the field is 'Reserved'. So, return error if we are on any other family/model.
>>>>
>>>> Impact on lm-sensors is minimal. On such families, instead of reporting
>>>> Current power value as '0', we now have:
>>>> power1: N/A
>>>>
>>> It will result in people complaining to us about it.
>>>
>>> It would be more appropriate to not create the attribute the first place
>>> if it is not supported. Sure, that is a bit more code, but it isn't that bad.
>>> You can simply return -ENODEV for unsupported CPUs from the probe function.
>>>
>>>
>>>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>>>> ---
>>>> drivers/hwmon/fam15h_power.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
>>>> index 4a7cbfa..b69bf7d 100644
>>>> --- a/drivers/hwmon/fam15h_power.c
>>>> +++ b/drivers/hwmon/fam15h_power.c
>>>> @@ -57,6 +57,10 @@ static ssize_t show_power(struct device *dev,
>>>> struct fam15h_power_data *data = dev_get_drvdata(dev);
>>>> struct pci_dev *f4 = data->pdev;
>>>> + /* The value TdpRunAvgAccCap is valid only on F15h, Models 0x0-0xF */
>>>> + if (boot_cpu_data.x86 != 0x15 || boot_cpu_data.x86_model > 0x0)
>>> The comment does not match the code. The comment talks about accepting models
>>> F15h, models 0x0-0xF, but the code rejects anything but F15h model 0x0.
>> Ah. Yes, The condition should have been (..boot_cpu_data.x86_model > 0xf)
>>
>>> Now it may well be that the above describes identifies all F15h and F16h CPUs,
>>> but this is not clear from the comment. It rather looks as if anything but F15h,
>>> model 0x0 is rejected, including all F16h CPUs. But then why accept F16h CPUs
>>> in the first place ?
>> Yes, we want to reject anything but F15h, Models 00h-0fh.
>> The reason I included the newer processor IDs, (and let
>> PCI_DEVICE_ID_AMD_16H_NB_F4) remain
>> is because we can still obtain 'critical power value'. It is only
>> the 'current power' that is not exposed.
>>
> That is a behavioral change, though; previously the current power was
> reported for F16h chips with PCI ID PCI_DEVICE_ID_AMD_16H_NB_F4.
> Is this a bug, ie should the power value not have been reported
> for the F16h chips ?
That's right.
>> If we return -ENODEV in the probe function (or we can just remove
>> the listed PCI_DEVICE_ID), then we'd not get the critical power
>> values too.
>>
> If you want to make the actual power reporting conditional, you should
> introduce an is_visible function to the attribute group to ensure that
> power1_input is only reported if/when supported. If the actual power
> value is not really supported for F16h chips, you should actually provide
> two separate patches: One to make power1_input optional, to be reported for
> supported chips only, and another to add more chips. One is a bug fix,
> the other a functionality extension.
>
Ok, I'll do that and resend.
Thanks,
-Aravind.
>
>>>> + return -ENOSYS;
>>>> +
>>>> pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
>>>> REG_TDP_RUNNING_AVERAGE, &val);
>>>> running_avg_capture = (val >> 4) & 0x3fffff;
>>>> @@ -216,7 +220,9 @@ static int fam15h_power_probe(struct pci_dev *pdev,
>>>> static const struct pci_device_id fam15h_power_id_table[] = {
>>>> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
>>>> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>>>> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>>>> + { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F3) },
>>>> {}
>>>> };
>>>> MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
>>>> --
>>>> 2.0.3
>>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-10 21:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-10 17:02 [PATCH] hwmon, fam15h_power: Add support for two more processors Aravind Gopalakrishnan
2014-09-10 17:53 ` Guenter Roeck
2014-09-10 20:01 ` Aravind Gopalakrishnan
2014-09-10 20:37 ` Guenter Roeck
2014-09-10 21:23 ` Aravind Gopalakrishnan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox