diff mbox series

[v3,3/8] cpufreq: amd-pstate: change driver to be built-in type

Message ID 20221107175705.2207842-4-Perry.Yuan@amd.com
State New
Headers
Series Implement AMD Pstate EPP Driver |

Commit Message

Yuan, Perry Nov. 7, 2022, 5:57 p.m. UTC 
  Change the `amd-pstate` driver as the built-in type which can help to
load the driver before the acpi_cpufreq driver as the default pstate
driver for the AMD processors.

for the processors do not have the dedicated MSR functions, add
`amd-pstate=legacy_cppc` to grub which enable shared memmory interface
to communicate with cppc_acpi module to control pstate hints.

Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Mario Limonciello Nov. 7, 2022, 6:29 p.m. UTC | #1 
+ wyes.karny@amd.com

You should sync with Wyes Karny on this patch, I think he had some 
different ideas that you guys should fold together for v4 of this 
series.  I'll leave some direct comments on your implementation below.

Also, include him in on CC for your v4.

On 11/7/2022 11:57, Perry Yuan wrote:
> Change the `amd-pstate` driver as the built-in type which can help to
> load the driver before the acpi_cpufreq driver as the default pstate
> driver for the AMD processors.
> 
> for the processors do not have the dedicated MSR functions, add
> `amd-pstate=legacy_cppc` to grub which enable shared memmory interface
> to communicate with cppc_acpi module to control pstate hints.

1) s/memmory/memory/
2) Although many users will use GRUB to configure their kernel command 
line you should not assume it in the commit message.

> 
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)

You need to document the new early parameter support in 
kernel-parameters.txt, and should also put it in amd-pstate.rst.

> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ace7d50cf2ac..14906431dc15 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -59,10 +59,7 @@
>    * we disable it by default to go acpi-cpufreq on these processors and add a
>    * module parameter to be able to enable it manually for debugging.
>    */
> -static bool shared_mem = false;
> -module_param(shared_mem, bool, 0444);
> -MODULE_PARM_DESC(shared_mem,
> -		 "enable amd-pstate on processors with shared memory solution (false = disabled (default), true = enabled)");
> +static bool shared_mem __read_mostly;
>   
>   static struct cpufreq_driver amd_pstate_driver;
>   
> @@ -653,16 +650,22 @@ static int __init amd_pstate_init(void)
>   
>   	return ret;
>   }
> +device_initcall(amd_pstate_init);
>   
> -static void __exit amd_pstate_exit(void)
> +static int __init amd_pstate_param(char *str)
>   {
> -	cpufreq_unregister_driver(&amd_pstate_driver);
> +	if (!str)
> +		return -EINVAL;
>   
> -	amd_pstate_enable(false);
> -}
> +	/* enable shared memory type CPPC ,if you processor has no MSR, you have to add this
> +	 * to your grub to make cppc driver loaded successfully.

Don't reference GRUB here, it should be referenced from the kernel 
command line.

> +	 */
> +	if (!strcmp(str, "legacy_cppc"))
> +		shared_mem = true;
Sync with Wyes about this.  He had some different strings and flow in 
mind which I think would be more preferable.

>   
> -module_init(amd_pstate_init);
> -module_exit(amd_pstate_exit);
> +	return 0;
> +}
> +early_param("amd-pstate", amd_pstate_param);
>   
>   MODULE_AUTHOR("Huang Rui <ray.huang@amd.com>");
>   MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");
Yuan, Perry Nov. 13, 2022, 4:21 p.m. UTC | #2 
[AMD Official Use Only - General]



> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, November 8, 2022 2:30 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com; Huang,
> Ray <Ray.Huang@amd.com>; viresh.kumar@linaro.org; Karny, Wyes
> <Wyes.Karny@amd.com>
> Cc: Sharma, Deepak <Deepak.Sharma@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in
> type
> 
> + wyes.karny@amd.com
> 
> You should sync with Wyes Karny on this patch, I think he had some different
> ideas that you guys should fold together for v4 of this series.  I'll leave some
> direct comments on your implementation below.
> 
> Also, include him in on CC for your v4.
> 
> On 11/7/2022 11:57, Perry Yuan wrote:
> > Change the `amd-pstate` driver as the built-in type which can help to
> > load the driver before the acpi_cpufreq driver as the default pstate
> > driver for the AMD processors.
> >
> > for the processors do not have the dedicated MSR functions, add
> > `amd-pstate=legacy_cppc` to grub which enable shared memmory interface
> > to communicate with cppc_acpi module to control pstate hints.
> 
> 1) s/memmory/memory/
> 2) Although many users will use GRUB to configure their kernel command
> line you should not assume it in the commit message.

Fixed in V4. 

> 
> >
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> >   drivers/cpufreq/amd-pstate.c | 23 +++++++++++++----------
> >   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> You need to document the new early parameter support in kernel-
> parameters.txt, and should also put it in amd-pstate.rst.
> 
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index ace7d50cf2ac..14906431dc15
> 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -59,10 +59,7 @@
> >    * we disable it by default to go acpi-cpufreq on these processors and add
> a
> >    * module parameter to be able to enable it manually for debugging.
> >    */
> > -static bool shared_mem = false;
> > -module_param(shared_mem, bool, 0444); -
> MODULE_PARM_DESC(shared_mem,
> > -		 "enable amd-pstate on processors with shared memory
> solution (false = disabled (default), true = enabled)");
> > +static bool shared_mem __read_mostly;
> >
> >   static struct cpufreq_driver amd_pstate_driver;
> >
> > @@ -653,16 +650,22 @@ static int __init amd_pstate_init(void)
> >
> >   	return ret;
> >   }
> > +device_initcall(amd_pstate_init);
> >
> > -static void __exit amd_pstate_exit(void)
> > +static int __init amd_pstate_param(char *str)
> >   {
> > -	cpufreq_unregister_driver(&amd_pstate_driver);
> > +	if (!str)
> > +		return -EINVAL;
> >
> > -	amd_pstate_enable(false);
> > -}
> > +	/* enable shared memory type CPPC ,if you processor has no MSR,
> you have to add this
> > +	 * to your grub to make cppc driver loaded successfully.
> 
> Don't reference GRUB here, it should be referenced from the kernel
> command line.
> 
> > +	 */
> > +	if (!strcmp(str, "legacy_cppc"))
> > +		shared_mem = true;
> Sync with Wyes about this.  He had some different strings and flow in mind
> which I think would be more preferable.

Aligned with Wyes, 
Some new changes brought  into V4.
Please help to take a look at the new patch.
Thanks

Perry. 


> 
> >
> > -module_init(amd_pstate_init);
> > -module_exit(amd_pstate_exit);
> > +	return 0;
> > +}
> > +early_param("amd-pstate", amd_pstate_param);
> >
> >   MODULE_AUTHOR("Huang Rui <ray.huang@amd.com>");
> >   MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index ace7d50cf2ac..14906431dc15 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -59,10 +59,7 @@ 
  * we disable it by default to go acpi-cpufreq on these processors and add a
  * module parameter to be able to enable it manually for debugging.
  */
-static bool shared_mem = false;
-module_param(shared_mem, bool, 0444);
-MODULE_PARM_DESC(shared_mem,
-		 "enable amd-pstate on processors with shared memory solution (false = disabled (default), true = enabled)");
+static bool shared_mem __read_mostly;
 
 static struct cpufreq_driver amd_pstate_driver;
 
@@ -653,16 +650,22 @@  static int __init amd_pstate_init(void)
 
 	return ret;
 }
+device_initcall(amd_pstate_init);
 
-static void __exit amd_pstate_exit(void)
+static int __init amd_pstate_param(char *str)
 {
-	cpufreq_unregister_driver(&amd_pstate_driver);
+	if (!str)
+		return -EINVAL;
 
-	amd_pstate_enable(false);
-}
+	/* enable shared memory type CPPC ,if you processor has no MSR, you have to add this
+	 * to your grub to make cppc driver loaded successfully.
+	 */
+	if (!strcmp(str, "legacy_cppc"))
+		shared_mem = true;
 
-module_init(amd_pstate_init);
-module_exit(amd_pstate_exit);
+	return 0;
+}
+early_param("amd-pstate", amd_pstate_param);
 
 MODULE_AUTHOR("Huang Rui <ray.huang@amd.com>");
 MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");