diff mbox series

[PATCHv3,6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

Message ID e48c6b836f996a16472c777612f1e3343c542077.1562781484.git.Janakarajan.Natarajan@amd.com (mailing list archive)
State Changes Requested, archived
Headers show
Series CPPC optional registers AMD support | expand

Commit Message

Janakarajan Natarajan July 10, 2019, 6:37 p.m. UTC
Add a new CPUFreq driver which exposes sysfs entries to control the
platform. To make use of this driver use a kernel commandline option.

Ex: amd_cpufreq=enable	- Enable AMD CPUFreq driver for Fam17h and later

Also, place amd-cpufreq before acpi-cpufreq in the Makefile to give it
higher priority.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 drivers/cpufreq/Kconfig.x86   |  14 ++
 drivers/cpufreq/Makefile      |   4 +-
 drivers/cpufreq/amd-cpufreq.c | 233 ++++++++++++++++++++++++++++++++++
 3 files changed, 250 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cpufreq/amd-cpufreq.c

Comments

Viresh Kumar July 11, 2019, 6:12 a.m. UTC | #1
On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
> diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
> +#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/vmalloc.h>
> +#include <linux/cpufreq.h>
> +#include <linux/acpi.h>
> +#include <linux/delay.h>

Please keep them in alphabetical order.

> +
> +#include <asm/unaligned.h>
> +
> +#include <acpi/cppc_acpi.h>
> +
> +struct amd_desc {
> +	int cpu_id;
> +	struct cppc_ctrls ctrls;
> +	struct kobject kobj;
> +};
> +
> +struct amd_desc **all_cpu_data;
> +
> +static unsigned int cppc_enable;
> +module_param(cppc_enable, uint, 0644);
> +MODULE_PARM_DESC(cppc_enable,
> +		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
> +
> +#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
> +
> +#define show_func(access_fn, struct_name, member_name)			\
> +	static ssize_t show_##member_name(struct kobject *kobj,		\
> +					  struct kobj_attribute *attr,	\
> +					  char *buf)			\
> +	{								\
> +		struct amd_desc *desc = to_amd_desc(kobj);		\
> +		struct struct_name st_name = {0};			\
> +		int ret;						\
> +									\
> +		ret = access_fn(desc->cpu_id, &st_name);		\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
> +				 (u64)st_name.member_name);		\
> +	}								\
> +
> +#define store_func(struct_name, member_name, reg_idx)			\
> +	static ssize_t store_##member_name(struct kobject *kobj,	\
> +					   struct kobj_attribute *attr,	\
> +					   const char *buf, size_t count)\
> +	{								\
> +		struct amd_desc *desc = to_amd_desc(kobj);		\
> +		struct struct_name st_name = {0};			\
> +		u32 val;						\
> +		int ret;						\
> +									\
> +		ret = kstrtou32(buf, 0, &val);				\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		st_name.member_name = val;				\
> +									\
> +		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
> +		if (ret)						\
> +			return ret;					\
> +									\
> +		return count;						\
> +	}								\
> +
> +#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
> +	show_func(access_fn, struct_name, member_name)			\
> +	store_func(struct_name, member_name, reg_idx)			\
> +	define_one_global_rw(member_name)
> +
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
> +define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
> +
> +static struct attribute *amd_cpufreq_attributes[] = {
> +	&enable.attr,
> +	&max_perf.attr,
> +	&min_perf.attr,
> +	&desired_perf.attr,
> +	&auto_sel_enable.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group amd_cpufreq_attr_group = {
> +	.attrs = amd_cpufreq_attributes,
> +};
> +
> +static struct kobj_type amd_cpufreq_type = {
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_attrs = amd_cpufreq_attributes,
> +};
> +
> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
> +{
> +	return 0;
> +}
> +
> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
> +					unsigned int index)
> +{
> +	return 0;
> +}

All empty helpers ? There is nothing you need to do ?

> +
> +static struct cpufreq_driver amd_cpufreq_driver = {
> +	.name = "amd_cpufreq",
> +	.init = amd_cpufreq_cpu_init,
> +	.exit = amd_cpufreq_cpu_exit,
> +	.verify = amd_cpufreq_cpu_verify,
> +	.target_index = amd_cpufreq_cpu_target_index,
> +};
> +
> +static void amd_cpufreq_sysfs_delete_params(void)
> +{
> +	int i;
> +
> +	for_each_possible_cpu(i) {
> +		if (all_cpu_data[i]) {
> +			kobject_del(&all_cpu_data[i]->kobj);

Shouldn't you use kobject_put() instead of this ?

> +			kfree(all_cpu_data[i]);
> +		}
> +	}
> +
> +	kfree(all_cpu_data);
> +}
> +
> +static int __init amd_cpufreq_sysfs_expose_params(void)
> +{
> +	struct device *cpu_dev;
> +	int i, ret;
> +
> +	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
> +			       GFP_KERNEL);
> +
> +	if (!all_cpu_data)
> +		return -ENOMEM;
> +
> +	for_each_possible_cpu(i) {
> +		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
> +		if (!all_cpu_data[i]) {
> +			ret = -ENOMEM;
> +			goto free;
> +		}
> +
> +		all_cpu_data[i]->cpu_id = i;
> +		cpu_dev = get_cpu_device(i);
> +		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
> +					   &cpu_dev->kobj, "amd_cpufreq");
> +		if (ret)
> +			goto free;
> +	}
> +
> +	return 0;
> +free:
> +	amd_cpufreq_sysfs_delete_params();
> +	return ret;
> +}

Instead of doing this once for all CPUs, it would be better to do it
every time the ->init() callback of the driver gets called. If you
have one cpufreq policy for each CPU (i.e. no CPUs share clock lines),
then the init() callback will get called once for each CPU.

> +
> +static int __init amd_cpufreq_init(void)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * Use only if:
> +	 * - AMD,
> +	 * - Family 17h (or) newer and,
> +	 * - Explicitly enabled
> +	 */
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
> +	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
> +		return -ENODEV;
> +
> +	ret = cpufreq_register_driver(&amd_cpufreq_driver);
> +	if (ret) {
> +		pr_info("Failed to register driver\n");
> +		goto out;
> +	}
> +
> +	ret = amd_cpufreq_sysfs_expose_params();
> +	if (ret) {
> +		pr_info("Could not create sysfs entries\n");
> +		cpufreq_unregister_driver(&amd_cpufreq_driver);
> +		goto out;
> +	}
> +
> +	pr_info("Using amd-cpufreq driver\n");
> +	return ret;
> +
> +out:
> +	return ret;
> +}
> +
> +static void __exit amd_cpufreq_exit(void)
> +{
> +	amd_cpufreq_sysfs_delete_params();
> +	cpufreq_unregister_driver(&amd_cpufreq_driver);
> +}
> +
> +static const struct acpi_device_id amd_acpi_ids[] __used = {
> +	{ACPI_PROCESSOR_DEVICE_HID, },
> +	{}
> +};
> +
> +device_initcall(amd_cpufreq_init);
> +module_exit(amd_cpufreq_exit);
> +MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);

All three should be placed directly below the struct/function they
represent without any blank lines in between. As suggested in
kernel documentation.

> +
> +MODULE_AUTHOR("Janakarajan Natarajan");
> +MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
> +MODULE_LICENSE("GPL");

Should this be "GPL v2" ?

> -- 
> 2.17.1
Janakarajan Natarajan July 11, 2019, 4:58 p.m. UTC | #2
On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>> diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
>> +#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/cpu.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
> Please keep them in alphabetical order.


Ok.


>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <acpi/cppc_acpi.h>
>> +
>> +struct amd_desc {
>> +	int cpu_id;
>> +	struct cppc_ctrls ctrls;
>> +	struct kobject kobj;
>> +};
>> +
>> +struct amd_desc **all_cpu_data;
>> +
>> +static unsigned int cppc_enable;
>> +module_param(cppc_enable, uint, 0644);
>> +MODULE_PARM_DESC(cppc_enable,
>> +		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
>> +
>> +#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
>> +
>> +#define show_func(access_fn, struct_name, member_name)			\
>> +	static ssize_t show_##member_name(struct kobject *kobj,		\
>> +					  struct kobj_attribute *attr,	\
>> +					  char *buf)			\
>> +	{								\
>> +		struct amd_desc *desc = to_amd_desc(kobj);		\
>> +		struct struct_name st_name = {0};			\
>> +		int ret;						\
>> +									\
>> +		ret = access_fn(desc->cpu_id, &st_name);		\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
>> +				 (u64)st_name.member_name);		\
>> +	}								\
>> +
>> +#define store_func(struct_name, member_name, reg_idx)			\
>> +	static ssize_t store_##member_name(struct kobject *kobj,	\
>> +					   struct kobj_attribute *attr,	\
>> +					   const char *buf, size_t count)\
>> +	{								\
>> +		struct amd_desc *desc = to_amd_desc(kobj);		\
>> +		struct struct_name st_name = {0};			\
>> +		u32 val;						\
>> +		int ret;						\
>> +									\
>> +		ret = kstrtou32(buf, 0, &val);				\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		st_name.member_name = val;				\
>> +									\
>> +		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		return count;						\
>> +	}								\
>> +
>> +#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
>> +	show_func(access_fn, struct_name, member_name)			\
>> +	store_func(struct_name, member_name, reg_idx)			\
>> +	define_one_global_rw(member_name)
>> +
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
>> +
>> +static struct attribute *amd_cpufreq_attributes[] = {
>> +	&enable.attr,
>> +	&max_perf.attr,
>> +	&min_perf.attr,
>> +	&desired_perf.attr,
>> +	&auto_sel_enable.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group amd_cpufreq_attr_group = {
>> +	.attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static struct kobj_type amd_cpufreq_type = {
>> +	.sysfs_ops = &kobj_sysfs_ops,
>> +	.default_attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>> +					unsigned int index)
>> +{
>> +	return 0;
>> +}
> All empty helpers ? There is nothing you need to do ?


When we posted v2 of this patchset, Rafael let us know that he was 
uncomfortable

going behind the (acpi-cpufreq) drivers back by letting the user 
communicate directly

with the platform. That's the reason we have an empty driver whose 
primary purpose

is to expose sysfs entries for the user.


>
>> +
>> +static struct cpufreq_driver amd_cpufreq_driver = {
>> +	.name = "amd_cpufreq",
>> +	.init = amd_cpufreq_cpu_init,
>> +	.exit = amd_cpufreq_cpu_exit,
>> +	.verify = amd_cpufreq_cpu_verify,
>> +	.target_index = amd_cpufreq_cpu_target_index,
>> +};
>> +
>> +static void amd_cpufreq_sysfs_delete_params(void)
>> +{
>> +	int i;
>> +
>> +	for_each_possible_cpu(i) {
>> +		if (all_cpu_data[i]) {
>> +			kobject_del(&all_cpu_data[i]->kobj);
> Shouldn't you use kobject_put() instead of this ?


Ok.


>
>> +			kfree(all_cpu_data[i]);
>> +		}
>> +	}
>> +
>> +	kfree(all_cpu_data);
>> +}
>> +
>> +static int __init amd_cpufreq_sysfs_expose_params(void)
>> +{
>> +	struct device *cpu_dev;
>> +	int i, ret;
>> +
>> +	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
>> +			       GFP_KERNEL);
>> +
>> +	if (!all_cpu_data)
>> +		return -ENOMEM;
>> +
>> +	for_each_possible_cpu(i) {
>> +		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
>> +		if (!all_cpu_data[i]) {
>> +			ret = -ENOMEM;
>> +			goto free;
>> +		}
>> +
>> +		all_cpu_data[i]->cpu_id = i;
>> +		cpu_dev = get_cpu_device(i);
>> +		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
>> +					   &cpu_dev->kobj, "amd_cpufreq");
>> +		if (ret)
>> +			goto free;
>> +	}
>> +
>> +	return 0;
>> +free:
>> +	amd_cpufreq_sysfs_delete_params();
>> +	return ret;
>> +}
> Instead of doing this once for all CPUs, it would be better to do it
> every time the ->init() callback of the driver gets called. If you
> have one cpufreq policy for each CPU (i.e. no CPUs share clock lines),
> then the init() callback will get called once for each CPU.


We are trying to avoid any use of the cpufreq mechanism.


Thanks,

Janakarajan


>
>> +
>> +static int __init amd_cpufreq_init(void)
>> +{
>> +	int ret = 0;
>> +
>> +	/*
>> +	 * Use only if:
>> +	 * - AMD,
>> +	 * - Family 17h (or) newer and,
>> +	 * - Explicitly enabled
>> +	 */
>> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
>> +	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
>> +		return -ENODEV;
>> +
>> +	ret = cpufreq_register_driver(&amd_cpufreq_driver);
>> +	if (ret) {
>> +		pr_info("Failed to register driver\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = amd_cpufreq_sysfs_expose_params();
>> +	if (ret) {
>> +		pr_info("Could not create sysfs entries\n");
>> +		cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +		goto out;
>> +	}
>> +
>> +	pr_info("Using amd-cpufreq driver\n");
>> +	return ret;
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +static void __exit amd_cpufreq_exit(void)
>> +{
>> +	amd_cpufreq_sysfs_delete_params();
>> +	cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +}
>> +
>> +static const struct acpi_device_id amd_acpi_ids[] __used = {
>> +	{ACPI_PROCESSOR_DEVICE_HID, },
>> +	{}
>> +};
>> +
>> +device_initcall(amd_cpufreq_init);
>> +module_exit(amd_cpufreq_exit);
>> +MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);
> All three should be placed directly below the struct/function they
> represent without any blank lines in between. As suggested in
> kernel documentation.
>
>> +
>> +MODULE_AUTHOR("Janakarajan Natarajan");
>> +MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
>> +MODULE_LICENSE("GPL");
> Should this be "GPL v2" ?
>
>> -- 
>> 2.17.1
Viresh Kumar July 12, 2019, 3:10 a.m. UTC | #3
On 11-07-19, 16:58, Janakarajan Natarajan wrote:
> On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> > On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
> >> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
> >> +					unsigned int index)
> >> +{
> >> +	return 0;
> >> +}
> > All empty helpers ? There is nothing you need to do ?
> 
> 
> When we posted v2 of this patchset, Rafael let us know that he was 
> uncomfortable
> 
> going behind the (acpi-cpufreq) drivers back by letting the user 
> communicate directly
> 
> with the platform. That's the reason we have an empty driver whose 
> primary purpose
> 
> is to expose sysfs entries for the user.

I read his comments now and what he suggested is:

"What about handling this like the others do, through a proper cpufreq
driver?"

I am not sure if he meant something like that you have here. Only one
cpufreq driver can be registered at any point of time with the kernel,
and so if this one is there then acpi-cpufreq or intel-pstate can't be
there. Who will do DVFS ?
Janakarajan Natarajan July 12, 2019, 4:44 p.m. UTC | #4
On 7/11/2019 10:10 PM, Viresh Kumar wrote:
> On 11-07-19, 16:58, Janakarajan Natarajan wrote:
>> On 7/11/2019 1:12 AM, Viresh Kumar wrote:
>>> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>>>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>>>> +					unsigned int index)
>>>> +{
>>>> +	return 0;
>>>> +}
>>> All empty helpers ? There is nothing you need to do ?
>>
>> When we posted v2 of this patchset, Rafael let us know that he was
>> uncomfortable
>>
>> going behind the (acpi-cpufreq) drivers back by letting the user
>> communicate directly
>>
>> with the platform. That's the reason we have an empty driver whose
>> primary purpose
>>
>> is to expose sysfs entries for the user.
> I read his comments now and what he suggested is:
>
> "What about handling this like the others do, through a proper cpufreq
> driver?"
>
> I am not sure if he meant something like that you have here. Only one
> cpufreq driver can be registered at any point of time with the kernel,
> and so if this one is there then acpi-cpufreq or intel-pstate can't be
> there. Who will do DVFS ?


By default, the driver is disabled and cpufreq falls back to using 
acpi-cpufreq. To enable the driver,

a parameter i.e. amd_cpufreq.cppc_enable=1 needs to be used. Then the 
user will gain access to

/sys/devices/system/cpu/cpu<num>/amd_cpufreq/{enable, max_perf, 
min_perf, desired_perf, auto_sel_enable}.


max_perf, min_perf allows the user to indicate to the platform the 
performance they expect

in an abstract scale [min_perf to max_perf] inclusive. desired_perf is 
the performance the user

would like to ideally achieve. desired_perf will fall withing max and 
min perf. Based on the value of these

registers, the platform will adjust a number of variables, voltage and 
frequency being one of them.


If the user would rather have the system handle all power/performance 
adjustments by

itself, then they can set the auto_sel_enable register.


Thanks,

Janakarajan
diff mbox series

Patch

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index dfa6457deaf6..01c7c5b5486a 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -32,6 +32,20 @@  config X86_PCC_CPUFREQ
 
 	  If in doubt, say N.
 
+config X86_AMD_CPUFREQ
+	tristate "AMD CPUFreq driver"
+	depends on ACPI_PROCESSOR
+	select ACPI_CPPC_LIB
+	help
+	  This adds a CPUFreq driver which uses CPPC methods
+	  as described in the ACPI v6.1 spec for newer (>= Fam17h)
+	  AMD processors.
+
+	  When this driver is enabled it will become preferred to
+	  the acpi-cpufreq driver.
+
+	  If in doubt, say N.
+
 config X86_ACPI_CPUFREQ
 	tristate "ACPI Processor P-States driver"
 	depends on ACPI_PROCESSOR
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 689b26c6f949..b2837ed9aff2 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -22,8 +22,10 @@  obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-dt-platdev.o
 # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
 # K8 systems. This is still the case but acpi-cpufreq errors out so that
 # powernow-k8 can load then. ACPI is preferred to all other hardware-specific drivers.
-# speedstep-* is preferred over p4-clockmod.
+# speedstep-* is preferred over p4-clockmod. amd-cpufreq is preferred to acpi-cpufreq
+# for Fam17h or newer AMD processors. For others, acpi-cpufreq will be used.
 
+obj-$(CONFIG_X86_AMD_CPUFREQ)		+= amd-cpufreq.o
 obj-$(CONFIG_X86_ACPI_CPUFREQ)		+= acpi-cpufreq.o
 obj-$(CONFIG_X86_POWERNOW_K8)		+= powernow-k8.o
 obj-$(CONFIG_X86_PCC_CPUFREQ)		+= pcc-cpufreq.o
diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
new file mode 100644
index 000000000000..262c8de3be2e
--- /dev/null
+++ b/drivers/cpufreq/amd-cpufreq.c
@@ -0,0 +1,233 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD CPUFREQ driver for Family 17h or greater AMD processors.
+ *
+ * Copyright (C) 2019 Advanced Micro Devices, Inc.
+ *
+ * Author: Janakarajan Natarajan <janakarajan.natarajan@amd.com>
+ */
+#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/vmalloc.h>
+#include <linux/cpufreq.h>
+#include <linux/acpi.h>
+#include <linux/delay.h>
+
+#include <asm/unaligned.h>
+
+#include <acpi/cppc_acpi.h>
+
+struct amd_desc {
+	int cpu_id;
+	struct cppc_ctrls ctrls;
+	struct kobject kobj;
+};
+
+struct amd_desc **all_cpu_data;
+
+static unsigned int cppc_enable;
+module_param(cppc_enable, uint, 0644);
+MODULE_PARM_DESC(cppc_enable,
+		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
+
+#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
+
+#define show_func(access_fn, struct_name, member_name)			\
+	static ssize_t show_##member_name(struct kobject *kobj,		\
+					  struct kobj_attribute *attr,	\
+					  char *buf)			\
+	{								\
+		struct amd_desc *desc = to_amd_desc(kobj);		\
+		struct struct_name st_name = {0};			\
+		int ret;						\
+									\
+		ret = access_fn(desc->cpu_id, &st_name);		\
+		if (ret)						\
+			return ret;					\
+									\
+		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
+				 (u64)st_name.member_name);		\
+	}								\
+
+#define store_func(struct_name, member_name, reg_idx)			\
+	static ssize_t store_##member_name(struct kobject *kobj,	\
+					   struct kobj_attribute *attr,	\
+					   const char *buf, size_t count)\
+	{								\
+		struct amd_desc *desc = to_amd_desc(kobj);		\
+		struct struct_name st_name = {0};			\
+		u32 val;						\
+		int ret;						\
+									\
+		ret = kstrtou32(buf, 0, &val);				\
+		if (ret)						\
+			return ret;					\
+									\
+		st_name.member_name = val;				\
+									\
+		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
+		if (ret)						\
+			return ret;					\
+									\
+		return count;						\
+	}								\
+
+#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
+	show_func(access_fn, struct_name, member_name)			\
+	store_func(struct_name, member_name, reg_idx)			\
+	define_one_global_rw(member_name)
+
+define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
+define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
+
+static struct attribute *amd_cpufreq_attributes[] = {
+	&enable.attr,
+	&max_perf.attr,
+	&min_perf.attr,
+	&desired_perf.attr,
+	&auto_sel_enable.attr,
+	NULL
+};
+
+static const struct attribute_group amd_cpufreq_attr_group = {
+	.attrs = amd_cpufreq_attributes,
+};
+
+static struct kobj_type amd_cpufreq_type = {
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_attrs = amd_cpufreq_attributes,
+};
+
+static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
+static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
+static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
+static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
+					unsigned int index)
+{
+	return 0;
+}
+
+static struct cpufreq_driver amd_cpufreq_driver = {
+	.name = "amd_cpufreq",
+	.init = amd_cpufreq_cpu_init,
+	.exit = amd_cpufreq_cpu_exit,
+	.verify = amd_cpufreq_cpu_verify,
+	.target_index = amd_cpufreq_cpu_target_index,
+};
+
+static void amd_cpufreq_sysfs_delete_params(void)
+{
+	int i;
+
+	for_each_possible_cpu(i) {
+		if (all_cpu_data[i]) {
+			kobject_del(&all_cpu_data[i]->kobj);
+			kfree(all_cpu_data[i]);
+		}
+	}
+
+	kfree(all_cpu_data);
+}
+
+static int __init amd_cpufreq_sysfs_expose_params(void)
+{
+	struct device *cpu_dev;
+	int i, ret;
+
+	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
+			       GFP_KERNEL);
+
+	if (!all_cpu_data)
+		return -ENOMEM;
+
+	for_each_possible_cpu(i) {
+		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
+		if (!all_cpu_data[i]) {
+			ret = -ENOMEM;
+			goto free;
+		}
+
+		all_cpu_data[i]->cpu_id = i;
+		cpu_dev = get_cpu_device(i);
+		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
+					   &cpu_dev->kobj, "amd_cpufreq");
+		if (ret)
+			goto free;
+	}
+
+	return 0;
+free:
+	amd_cpufreq_sysfs_delete_params();
+	return ret;
+}
+
+static int __init amd_cpufreq_init(void)
+{
+	int ret = 0;
+
+	/*
+	 * Use only if:
+	 * - AMD,
+	 * - Family 17h (or) newer and,
+	 * - Explicitly enabled
+	 */
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
+	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
+		return -ENODEV;
+
+	ret = cpufreq_register_driver(&amd_cpufreq_driver);
+	if (ret) {
+		pr_info("Failed to register driver\n");
+		goto out;
+	}
+
+	ret = amd_cpufreq_sysfs_expose_params();
+	if (ret) {
+		pr_info("Could not create sysfs entries\n");
+		cpufreq_unregister_driver(&amd_cpufreq_driver);
+		goto out;
+	}
+
+	pr_info("Using amd-cpufreq driver\n");
+	return ret;
+
+out:
+	return ret;
+}
+
+static void __exit amd_cpufreq_exit(void)
+{
+	amd_cpufreq_sysfs_delete_params();
+	cpufreq_unregister_driver(&amd_cpufreq_driver);
+}
+
+static const struct acpi_device_id amd_acpi_ids[] __used = {
+	{ACPI_PROCESSOR_DEVICE_HID, },
+	{}
+};
+
+device_initcall(amd_cpufreq_init);
+module_exit(amd_cpufreq_exit);
+MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);
+
+MODULE_AUTHOR("Janakarajan Natarajan");
+MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
+MODULE_LICENSE("GPL");