Re: [lm-sensors] [PATCH] hwmon: fam15h_power: fix bogus values with current BIOSes

[Andre Przywara <andre.przywara@amd.com>]

On 04/09/2012 07:39 PM, Jean Delvare wrote:
> On Sun, 8 Apr 2012 01:07:59 +0200, Andre Przywara wrote:
>> Newer BKDG[1] versions recommend a different initialization value for
>> the running average range register in the northbridge. This improves
>> the power reading by avoiding counter saturations resulting in bogus
>> values for anything below about 80% of TDP power consumption.
>> Updated BIOSes will have this new value set up from the beginning,
>> but meanwhile we correct this value ourselves.
>> This needs to be done on all northbridges, even on those where the
>> driver itself does not register at.
>>
>> This fixes the driver on all current machines to provide proper
>> values for idle load.
>>
>> [1]
>> http://support.amd.com/us/Processor_TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf
>> Chapter 3.8: D18F5xE0 Processor TDP Running Average (p. 452)
>>
>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>> ---
>>   drivers/hwmon/fam15h_power.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
>> index b7494af..2aa7d9d 100644
>> --- a/drivers/hwmon/fam15h_power.c
>> +++ b/drivers/hwmon/fam15h_power.c
>> @@ -122,6 +122,39 @@ static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
>>   	return true;
>>   }
>>
>> +/*
>> + * Newer BKDG versions have an updated recommendation on how to properly
>> + * initialize the running average range (was: 0xE, now: 0x9). This avoids
>> + * counter saturations resulting in bogus power readings.
>> + * We correct this value ourselves to cope with older BIOSes.
>> + */
>> +static int tweak_runavg_range(struct pci_dev *pdev)
>
> Can be marked __devinit.
>
>> +{
>> +	u32 val;
>> +	struct pci_device_id affected_device = {
>> +		PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) };
>
> AFAICS this can be declared const.
>
>> +
>> +	/*
>> +	 * let this quirk apply only to the current version of the
>> +	 * northbridge, since future versions may change the behavior
>> +	 */
>> +	if (!pci_match_id(&affected_device, pdev))
>> +		return 0;
>> +
>> +	pci_bus_read_config_dword(pdev->bus,
>> +		PCI_DEVFN(PCI_SLOT(pdev->devfn), 5),
>> +		REG_TDP_RUNNING_AVERAGE,&val);
>> +	if ((val&  0xf) != 0xe)
>> +		return 0;
>> +
>> +	val&= ~0x0f;
>> +	val |= 9;
>
> Please don't mix decimal values, one-digit hexadecimal and two-digit
> hexadecimal values. Choose one format and stick to it.
>
> I am a little curious about the code BTW, as the value you write isn't
> the one you originally checked for, which means that reloading the
> driver would have to do it again...

The whole code is about to fix a BIOS flaw. It needs to be done only 
once after each reset, newer BIOSes will do this at boot time already. 
So reloading the driver will not fix this again, because it is not 
needed. I check against the 0xe value to avoid breaking future BIOSes, 
which may write other values than 0x9 in this register.
This could also be done in a PCI quirk, but I refrained from this 
because I don't want to tinker with BIOS values when the actual driver 
is not needed.
Fixed the other comments.

Thanks for looking at the code,
Andre.

>
>> +	pci_bus_write_config_dword(pdev->bus,
>> +		PCI_DEVFN(PCI_SLOT(pdev->devfn), 5),
>> +		REG_TDP_RUNNING_AVERAGE, val);
>> +	return 1;
>> +}
>> +
>>   static void __devinit fam15h_power_init_data(struct pci_dev *f4,
>>   					     struct fam15h_power_data *data)
>>   {
>> @@ -155,6 +188,13 @@ static int __devinit fam15h_power_probe(struct pci_dev *pdev,
>>   	struct device *dev;
>>   	int err;
>>
>> +	/*
>> +	 * though we ignore every other northbridge, we still have to
>> +	 * do the tweaking on _each_ node in MCM processors as the counters
>> +	 * are working hand-in-hand
>> +	 */
>> +	tweak_runavg_range(pdev);
>> +
>>   	if (!fam15h_power_is_internal_node0(pdev)) {
>>   		err = -ENODEV;
>>   		goto exit;
>
>


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors