* [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