lkml.org 
[lkml]   [2022]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in type
From
+ 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");

\
 
 \ /
  Last update: 2022-11-07 19:30    [W:0.613 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site