* Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume
2012-09-20 10:34 [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume Andreas Herrmann
@ 2012-09-20 16:26 ` Andreas Hartmann
2012-09-20 16:35 ` Guenter Roeck
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Andreas Hartmann @ 2012-09-20 16:26 UTC (permalink / raw)
To: lm-sensors
Hello Andreas,
Andreas Herrmann wrote:
>
> The quirk introduced with commit
> 00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
> bogus values with current BIOSes) is not only required during driver
> load but also when system resumes from suspend. The BIOS might set the
> previously recommended (but unsuitable) initilization value for the
> running average range register during resume.
>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
I could test your patch successfully with kernel 3.4.11. The reported
power consumption
fam15h_power-pci-00c4
Adapter: PCI adapter
power1: 28.45 W
works now as expected.
Thanks,
kind regards,
Andreas Hartmann
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume
2012-09-20 10:34 [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume Andreas Herrmann
2012-09-20 16:26 ` Andreas Hartmann
@ 2012-09-20 16:35 ` Guenter Roeck
2012-09-23 17:06 ` Jean Delvare
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2012-09-20 16:35 UTC (permalink / raw)
To: lm-sensors
On Thu, Sep 20, 2012 at 12:34:08PM +0200, Andreas Herrmann wrote:
>
> The quirk introduced with commit
> 00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
> bogus values with current BIOSes) is not only required during driver
> load but also when system resumes from suspend. The BIOS might set the
> previously recommended (but unsuitable) initilization value for the
> running average range register during resume.
>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
> drivers/hwmon/fam15h_power.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> Hi Jean,
>
> Thanks for spotting the issues.
> Here's a new patch version, esp. to fix the section mismatch.
>
> Please apply.
>
>
> Thanks,
>
> Andreas
>
> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> index 2764b78a784b..af69073b3fe8 100644
> --- a/drivers/hwmon/fam15h_power.c
> +++ b/drivers/hwmon/fam15h_power.c
> @@ -129,12 +129,12 @@ static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
> * counter saturations resulting in bogus power readings.
> * We correct this value ourselves to cope with older BIOSes.
> */
> -static DEFINE_PCI_DEVICE_TABLE(affected_device) = {
> +static const struct pci_device_id affected_device[] = {
I would prefer if we could keep DEFINE_PCI_DEVICE_TABLE to avoid the
checkpatch warning. Should be easy to implement, by moving pci_match_id
into the probe function and only call tweak_runavg_range() if it returns true.
You could keep the response in fam15h_power_data, as a flag such as needs_tweak,
for use in fam15h_power_resume().
Just my $0.02, though. I'll leave it up to Jean to decide if the patch is ok
as-is.
Guenter
> { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> { 0 }
> };
>
> -static void __devinit tweak_runavg_range(struct pci_dev *pdev)
> +static void tweak_runavg_range(struct pci_dev *pdev)
> {
> u32 val;
>
> @@ -158,6 +158,16 @@ static void __devinit tweak_runavg_range(struct pci_dev *pdev)
> REG_TDP_RUNNING_AVERAGE, val);
> }
>
> +#ifdef CONFIG_PM
> +static int fam15h_power_resume(struct pci_dev *pdev)
> +{
> + tweak_runavg_range(pdev);
> + return 0;
> +}
> +#else
> +#define fam15h_power_resume NULL
> +#endif
> +
> static void __devinit fam15h_power_init_data(struct pci_dev *f4,
> struct fam15h_power_data *data)
> {
> @@ -256,6 +266,7 @@ static struct pci_driver fam15h_power_driver = {
> .id_table = fam15h_power_id_table,
> .probe = fam15h_power_probe,
> .remove = __devexit_p(fam15h_power_remove),
> + .resume = fam15h_power_resume,
> };
>
> module_pci_driver(fam15h_power_driver);
> --
> 1.7.10.4
>
>
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume
2012-09-20 10:34 [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume Andreas Herrmann
2012-09-20 16:26 ` Andreas Hartmann
2012-09-20 16:35 ` Guenter Roeck
@ 2012-09-23 17:06 ` Jean Delvare
2013-07-18 14:47 ` Jean Delvare
2013-07-18 14:51 ` Satoshi Imamura
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2012-09-23 17:06 UTC (permalink / raw)
To: lm-sensors
On Thu, 20 Sep 2012 09:35:39 -0700, Guenter Roeck wrote:
> On Thu, Sep 20, 2012 at 12:34:08PM +0200, Andreas Herrmann wrote:
> >
> > The quirk introduced with commit
> > 00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
> > bogus values with current BIOSes) is not only required during driver
> > load but also when system resumes from suspend. The BIOS might set the
> > previously recommended (but unsuitable) initilization value for the
> > running average range register during resume.
> >
> > Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> > ---
> > drivers/hwmon/fam15h_power.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > Hi Jean,
> >
> > Thanks for spotting the issues.
> > Here's a new patch version, esp. to fix the section mismatch.
> >
> > Please apply.
> >
> >
> > Thanks,
> >
> > Andreas
> >
> > diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> > index 2764b78a784b..af69073b3fe8 100644
> > --- a/drivers/hwmon/fam15h_power.c
> > +++ b/drivers/hwmon/fam15h_power.c
> > @@ -129,12 +129,12 @@ static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
> > * counter saturations resulting in bogus power readings.
> > * We correct this value ourselves to cope with older BIOSes.
> > */
> > -static DEFINE_PCI_DEVICE_TABLE(affected_device) = {
> > +static const struct pci_device_id affected_device[] = {
>
> I would prefer if we could keep DEFINE_PCI_DEVICE_TABLE to avoid the
> checkpatch warning. Should be easy to implement, by moving pci_match_id
> into the probe function and only call tweak_runavg_range() if it returns true.
> You could keep the response in fam15h_power_data, as a flag such as needs_tweak,
> for use in fam15h_power_resume().
>
> Just my $0.02, though. I'll leave it up to Jean to decide if the patch is ok
> as-is.
I'll apply the patch as is. The warning from checkpatch is a false
positive, it happens sometimes and that's no reason for changing
working, tested code. Especially for a code change I'll push upstream
right before a release, and which should go to stable kernels, keeping
the patch simple is a must.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume
2012-09-20 10:34 [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume Andreas Herrmann
` (2 preceding siblings ...)
2012-09-23 17:06 ` Jean Delvare
@ 2013-07-18 14:47 ` Jean Delvare
2013-07-18 14:51 ` Satoshi Imamura
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2013-07-18 14:47 UTC (permalink / raw)
To: lm-sensors
Hi Satoshi,
On Wed, 17 Jul 2013 21:35:43 +0000 (UTC), Satoshi Imamura wrote:
> Hello Mr. Andreas Herrmann,
I don't think Andreas is still reading the lm-sensors list, so I am
adding him to Cc.
> I'm trying to measure power consumption of AMD Opteron 6272 using fam15h_power.
> Linux kernel version is 3.9.4.
> Even when all cores are idle, fam15h_power shows about 85W.
> However, the power consumption decreases to about 74W
> when a process is executed with all cores.
>
> For Linux kernel 3.9.4, the patch for fam15h_power is already applied.
> Moreover, "RunAvgRange" (p. 460 of BKDG for Family 15h) is
> appropriately set to "0x9" as following:
>
> % setpci -s 00:18.5 0xe0.l
> % 00372c99
>
> Why is the power consumption so high (85W) when all cores are idle?
> Why does it decrease when a process is executed with all cores?
> I would appreciate if you could give me a advice.
>
> Sincerely yours,
>
> Satoshi
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume
2012-09-20 10:34 [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume Andreas Herrmann
` (3 preceding siblings ...)
2013-07-18 14:47 ` Jean Delvare
@ 2013-07-18 14:51 ` Satoshi Imamura
4 siblings, 0 replies; 6+ messages in thread
From: Satoshi Imamura @ 2013-07-18 14:51 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Thank you very much for your cooperation.
Regards,
Satoshi
--
Satoshi Imamura
Visiting Researcher
School of Electronics, Electrical Engineering and Computer Science
Queen's University of Belfast
Email: s.imamura@qub.ac.uk
On Jul 18, 2013, at 3:47 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Satoshi,
>
> On Wed, 17 Jul 2013 21:35:43 +0000 (UTC), Satoshi Imamura wrote:
>> Hello Mr. Andreas Herrmann,
>
> I don't think Andreas is still reading the lm-sensors list, so I am
> adding him to Cc.
>
>> I'm trying to measure power consumption of AMD Opteron 6272 using fam15h_power.
>> Linux kernel version is 3.9.4.
>> Even when all cores are idle, fam15h_power shows about 85W.
>> However, the power consumption decreases to about 74W
>> when a process is executed with all cores.
>>
>> For Linux kernel 3.9.4, the patch for fam15h_power is already applied.
>> Moreover, "RunAvgRange" (p. 460 of BKDG for Family 15h) is
>> appropriately set to "0x9" as following:
>>
>> % setpci -s 00:18.5 0xe0.l
>> % 00372c99
>>
>> Why is the power consumption so high (85W) when all cores are idle?
>> Why does it decrease when a process is executed with all cores?
>> I would appreciate if you could give me a advice.
>>
>> Sincerely yours,
>>
>> Satoshi
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread