+ [email protected]
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 <[email protected]>
> ---
> 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 <[email protected]>");
> MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");
[AMD Official Use Only - General]
> -----Original Message-----
> From: Limonciello, Mario <[email protected]>
> Sent: Tuesday, November 8, 2022 2:30 AM
> To: Yuan, Perry <[email protected]>; [email protected]; Huang,
> Ray <[email protected]>; [email protected]; Karny, Wyes
> <[email protected]>
> Cc: Sharma, Deepak <[email protected]>; Fontenot, Nathan
> <[email protected]>; Deucher, Alexander
> <[email protected]>; Huang, Shimmer
> <[email protected]>; Du, Xiaojian <[email protected]>; Meng,
> Li (Jassmine) <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v3 3/8] cpufreq: amd-pstate: change driver to be built-in
> type
>
> + [email protected]
>
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>");
> > MODULE_DESCRIPTION("AMD Processor P-state Frequency Driver");