[Patch 0/2] Add support for _TFP and configurable throttle pctg

[Patch 0/2] Add support for _TFP and configurable throttle pctg

[Sumit Gupta]

This patch set adds support for two features to get a finer control
over the impact of Thermal Throttling on performance.

 1) Patch 1: Adds support to read Thermal fast Sampling Period (_TFP)
    ACPI object and use it over "Thermal Sampling Period (_TSP)" for
    Passive cooling if both are present.

 2) Patch 2: Adds support to configure the CPUFREQ reduction percentage
    and not always cause throttling in steps of "20%".

Both patches can be applied independently.

Jeff Brasen (1):
  ACPI: thermal: Add Thermal fast Sampling Period (_TFP) support

Srikar Srimath Tirumala (1):
  ACPI: processor: Add support to configure CPUFREQ reduction pctg

 drivers/acpi/Kconfig             | 15 +++++++++++++++
 drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
 drivers/acpi/thermal.c           | 21 ++++++++++++++-------
 3 files changed, 45 insertions(+), 10 deletions(-)

-- 
2.17.1

[Patch 1/2] ACPI: thermal: Add Thermal fast Sampling Period (_TFP) support

[Sumit Gupta]

From: Jeff Brasen <jbrasen@nvidia.com>

Add support for Thermal fast Sampling Period (_TFP) for Passive cooling.
As per UEFI spec, _TFP overrides the "Thermal Sampling Period (_TSP)"
if both are present in a Thermal zone.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/acpi/thermal.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index f9f6ebb08fdb..5dee3722509c 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -111,7 +111,7 @@ struct acpi_thermal_passive {
 	unsigned long temperature;
 	unsigned long tc1;
 	unsigned long tc2;
-	unsigned long tsp;
+	unsigned long sampling_period;
 	bool valid;
 };
 
@@ -289,11 +289,18 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
 					tz->trips.passive.tc2 = tmp;
 
 				status = acpi_evaluate_integer(tz->device->handle,
-							       "_TSP", NULL, &tmp);
-				if (ACPI_FAILURE(status))
-					tz->trips.passive.valid = false;
-				else
-					tz->trips.passive.tsp = tmp;
+							       "_TFP", NULL, &tmp);
+				if (ACPI_FAILURE(status)) {
+					status = acpi_evaluate_integer(tz->device->handle,
+								       "_TSP", NULL, &tmp);
+					if (ACPI_FAILURE(status))
+						tz->trips.passive.valid = false;
+					else
+						tz->trips.passive.sampling_period = tmp * 100;
+
+				} else {
+					tz->trips.passive.sampling_period = tmp;
+				}
 			}
 		}
 	}
@@ -765,7 +772,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
 	if (tz->trips.passive.valid)
 		tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz,
 								&acpi_thermal_zone_ops, NULL,
-								tz->trips.passive.tsp * 100,
+								tz->trips.passive.sampling_period,
 								tz->polling_frequency * 100);
 	else
 		tz->thermal_zone =
-- 
2.17.1

Re: [Patch 1/2] ACPI: thermal: Add Thermal fast Sampling Period (_TFP) support

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 11:30 AM Sumit Gupta <sumitg@nvidia.com> wrote:
>
> From: Jeff Brasen <jbrasen@nvidia.com>
>
> Add support for Thermal fast Sampling Period (_TFP) for Passive cooling.
> As per UEFI spec,

You mean the ACPI spec I suppose?  It would be good to give the
relevant section number and title.

>  _TFP overrides the "Thermal Sampling Period (_TSP)"
>  if both are present in a Thermal zone.
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
>  drivers/acpi/thermal.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index f9f6ebb08fdb..5dee3722509c 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -111,7 +111,7 @@ struct acpi_thermal_passive {
>         unsigned long temperature;
>         unsigned long tc1;
>         unsigned long tc2;
> -       unsigned long tsp;
> +       unsigned long sampling_period;
>         bool valid;
>  };
>
> @@ -289,11 +289,18 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
>                                         tz->trips.passive.tc2 = tmp;
>
>                                 status = acpi_evaluate_integer(tz->device->handle,
> -                                                              "_TSP", NULL, &tmp);
> -                               if (ACPI_FAILURE(status))
> -                                       tz->trips.passive.valid = false;
> -                               else
> -                                       tz->trips.passive.tsp = tmp;
> +                                                              "_TFP", NULL, &tmp);
> +                               if (ACPI_FAILURE(status)) {
> +                                       status = acpi_evaluate_integer(tz->device->handle,
> +                                                                      "_TSP", NULL, &tmp);
> +                                       if (ACPI_FAILURE(status))
> +                                               tz->trips.passive.valid = false;
> +                                       else
> +                                               tz->trips.passive.sampling_period = tmp * 100;
> +
> +                               } else {
> +                                       tz->trips.passive.sampling_period = tmp;
> +                               }
>                         }
>                 }
>         }
> @@ -765,7 +772,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
>         if (tz->trips.passive.valid)
>                 tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz,
>                                                                 &acpi_thermal_zone_ops, NULL,
> -                                                               tz->trips.passive.tsp * 100,
> +                                                               tz->trips.passive.sampling_period,
>                                                                 tz->polling_frequency * 100);
>         else
>                 tz->thermal_zone =
> --

So this needs to be rebased on top of the current linux-next branch in
linux-pm.git or on top of the acpi-thermal branch in there.

Re: [Patch 1/2] ACPI: thermal: Add Thermal fast Sampling Period (_TFP) support

[Sumit Gupta]

On 19/08/23 00:03, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Aug 17, 2023 at 11:30 AM Sumit Gupta <sumitg@nvidia.com> wrote:
>>
>> From: Jeff Brasen <jbrasen@nvidia.com>
>>
>> Add support for Thermal fast Sampling Period (_TFP) for Passive cooling.
>> As per UEFI spec,
> 
> You mean the ACPI spec I suppose?  It would be good to give the
> relevant section number and title.
> 

Yes, It's the ACPI Spec 6.4. Section and title are "11.4.17. _TFP 
(Thermal fast Sampling Period)". I will add it in description in v2.
Thank you for correcting.

>>   _TFP overrides the "Thermal Sampling Period (_TSP)"
>>   if both are present in a Thermal zone.
>>
>> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/acpi/thermal.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>> index f9f6ebb08fdb..5dee3722509c 100644
>> --- a/drivers/acpi/thermal.c
>> +++ b/drivers/acpi/thermal.c
>> @@ -111,7 +111,7 @@ struct acpi_thermal_passive {
>>          unsigned long temperature;
>>          unsigned long tc1;
>>          unsigned long tc2;
>> -       unsigned long tsp;
>> +       unsigned long sampling_period;
>>          bool valid;
>>   };
>>
>> @@ -289,11 +289,18 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
>>                                          tz->trips.passive.tc2 = tmp;
>>
>>                                  status = acpi_evaluate_integer(tz->device->handle,
>> -                                                              "_TSP", NULL, &tmp);
>> -                               if (ACPI_FAILURE(status))
>> -                                       tz->trips.passive.valid = false;
>> -                               else
>> -                                       tz->trips.passive.tsp = tmp;
>> +                                                              "_TFP", NULL, &tmp);
>> +                               if (ACPI_FAILURE(status)) {
>> +                                       status = acpi_evaluate_integer(tz->device->handle,
>> +                                                                      "_TSP", NULL, &tmp);
>> +                                       if (ACPI_FAILURE(status))
>> +                                               tz->trips.passive.valid = false;
>> +                                       else
>> +                                               tz->trips.passive.sampling_period = tmp * 100;
>> +
>> +                               } else {
>> +                                       tz->trips.passive.sampling_period = tmp;
>> +                               }
>>                          }
>>                  }
>>          }
>> @@ -765,7 +772,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
>>          if (tz->trips.passive.valid)
>>                  tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz,
>>                                                                  &acpi_thermal_zone_ops, NULL,
>> -                                                               tz->trips.passive.tsp * 100,
>> +                                                               tz->trips.passive.sampling_period,
>>                                                                  tz->polling_frequency * 100);
>>          else
>>                  tz->thermal_zone =
>> --
> 
> So this needs to be rebased on top of the current linux-next branch in
> linux-pm.git or on top of the acpi-thermal branch in there.

Will rebase and send v2.

Thank you,
Sumit Gupta

[Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Sumit Gupta]

From: Srikar Srimath Tirumala <srikars@nvidia.com>

Add support to configure the CPUFREQ reduction percentage and set the
maximum number of throttling steps accordingly. Current implementation
of processor_thermal performs software throttling in fixed steps of
"20%" which can be too coarse for some platforms. Change that by adding
new config to provide the reduction percentage.

Signed-off-by: Srikar Srimath Tirumala <srikars@nvidia.com>
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/acpi/Kconfig             | 15 +++++++++++++++
 drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 00dd309b6682..287cf58defbf 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -254,6 +254,21 @@ config ACPI_DOCK
 config ACPI_CPU_FREQ_PSS
 	bool
 
+config ACPI_CPU_FREQ_THERM_HAS_PARAMS
+	bool "CPU frequency throttling control"
+	depends on ACPI_PROCESSOR
+
+config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
+	int "Minimum throttle percentage for processor_thermal cooling device"
+	depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
+	default 20
+	help
+	  The processor_thermal driver uses this config to calculate the
+	  percentage amount by which cpu frequency must be reduced for each
+	  cooling state. The config is also used to calculate the maximum number
+	  of throttling steps or cooling states supported by the driver. Value
+	  must be an unsigned integer in the range [1, 50].
+
 config ACPI_PROCESSOR_CSTATE
 	def_bool y
 	depends on ACPI_PROCESSOR
diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index b7c6287eccca..ee443cc69b73 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -25,8 +25,20 @@
  * _any_ cpufreq driver and not only the acpi-cpufreq driver.
  */
 
-#define CPUFREQ_THERMAL_MIN_STEP 0
-#define CPUFREQ_THERMAL_MAX_STEP 3
+#define CPUFREQ_THERMAL_MIN_STEP	0
+#ifdef CONFIG_ACPI_CPU_FREQ_THERM_HAS_PARAMS
+#define CPUFREQ_THERMAL_PCTG		CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
+
+/* Derive the MAX_STEP from minimum throttle percentage so that the reduction
+ * percentage does end up becoming negative. Also cap the MAX_STEP so that
+ * the CPU performance doesn't become 0.
+ */
+#define CPUFREQ_THERMAL_MAX_STEP	((100 / CPUFREQ_THERMAL_PCTG) - 1)
+
+#else
+#define CPUFREQ_THERMAL_MAX_STEP	3
+#define CPUFREQ_THERMAL_PCTG		20
+#endif
 
 static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
 
@@ -113,7 +125,8 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
 		if (!policy)
 			return -EINVAL;
 
-		max_freq = (policy->cpuinfo.max_freq * (100 - reduction_pctg(i) * 20)) / 100;
+		max_freq = (policy->cpuinfo.max_freq *
+			    (100 - reduction_pctg(i) * CPUFREQ_THERMAL_PCTG)) / 100;
 
 		cpufreq_cpu_put(policy);
 
-- 
2.17.1

Re: [Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 11:31 AM Sumit Gupta <sumitg@nvidia.com> wrote:
>
> From: Srikar Srimath Tirumala <srikars@nvidia.com>
>
> Add support to configure the CPUFREQ reduction percentage and set the
> maximum number of throttling steps accordingly. Current implementation
> of processor_thermal performs software throttling in fixed steps of
> "20%" which can be too coarse for some platforms. Change that by adding
> new config to provide the reduction percentage.
>
> Signed-off-by: Srikar Srimath Tirumala <srikars@nvidia.com>
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
>  drivers/acpi/Kconfig             | 15 +++++++++++++++
>  drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
>  2 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 00dd309b6682..287cf58defbf 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -254,6 +254,21 @@ config ACPI_DOCK
>  config ACPI_CPU_FREQ_PSS
>         bool
>
> +config ACPI_CPU_FREQ_THERM_HAS_PARAMS
> +       bool "CPU frequency throttling control"
> +       depends on ACPI_PROCESSOR
> +
> +config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> +       int "Minimum throttle percentage for processor_thermal cooling device"
> +       depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
> +       default 20
> +       help
> +         The processor_thermal driver uses this config to calculate the
> +         percentage amount by which cpu frequency must be reduced for each
> +         cooling state. The config is also used to calculate the maximum number
> +         of throttling steps or cooling states supported by the driver. Value
> +         must be an unsigned integer in the range [1, 50].
> +

I don't think that the new Kconfig symbols are particularly useful.
At least they don't help the distro vendors that each would need to
pick up a specific value for their kernel anyway.

I also wonder how the users building their own kernels are supposed to
determine the values suitable for the target systems.

>  config ACPI_PROCESSOR_CSTATE
>         def_bool y
>         depends on ACPI_PROCESSOR
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index b7c6287eccca..ee443cc69b73 100644
> --- a/drivers/acpi/processor_thermal.c
> +++ b/drivers/acpi/processor_thermal.c
> @@ -25,8 +25,20 @@
>   * _any_ cpufreq driver and not only the acpi-cpufreq driver.
>   */
>
> -#define CPUFREQ_THERMAL_MIN_STEP 0
> -#define CPUFREQ_THERMAL_MAX_STEP 3
> +#define CPUFREQ_THERMAL_MIN_STEP       0
> +#ifdef CONFIG_ACPI_CPU_FREQ_THERM_HAS_PARAMS
> +#define CPUFREQ_THERMAL_PCTG           CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> +
> +/* Derive the MAX_STEP from minimum throttle percentage so that the reduction
> + * percentage does end up becoming negative. Also cap the MAX_STEP so that
> + * the CPU performance doesn't become 0.
> + */
> +#define CPUFREQ_THERMAL_MAX_STEP       ((100 / CPUFREQ_THERMAL_PCTG) - 1)
> +
> +#else
> +#define CPUFREQ_THERMAL_MAX_STEP       3
> +#define CPUFREQ_THERMAL_PCTG           20
> +#endif
>
>  static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
>
> @@ -113,7 +125,8 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
>                 if (!policy)
>                         return -EINVAL;
>
> -               max_freq = (policy->cpuinfo.max_freq * (100 - reduction_pctg(i) * 20)) / 100;
> +               max_freq = (policy->cpuinfo.max_freq *
> +                           (100 - reduction_pctg(i) * CPUFREQ_THERMAL_PCTG)) / 100;
>
>                 cpufreq_cpu_put(policy);
>
> --

Re: [Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Sumit Gupta]

On 19/08/23 00:10, Rafael J. Wysocki wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Aug 17, 2023 at 11:31 AM Sumit Gupta <sumitg@nvidia.com> wrote:
>>
>> From: Srikar Srimath Tirumala <srikars@nvidia.com>
>>
>> Add support to configure the CPUFREQ reduction percentage and set the
>> maximum number of throttling steps accordingly. Current implementation
>> of processor_thermal performs software throttling in fixed steps of
>> "20%" which can be too coarse for some platforms. Change that by adding
>> new config to provide the reduction percentage.
>>
>> Signed-off-by: Srikar Srimath Tirumala <srikars@nvidia.com>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/acpi/Kconfig             | 15 +++++++++++++++
>>   drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
>>   2 files changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 00dd309b6682..287cf58defbf 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -254,6 +254,21 @@ config ACPI_DOCK
>>   config ACPI_CPU_FREQ_PSS
>>          bool
>>
>> +config ACPI_CPU_FREQ_THERM_HAS_PARAMS
>> +       bool "CPU frequency throttling control"
>> +       depends on ACPI_PROCESSOR
>> +
>> +config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
>> +       int "Minimum throttle percentage for processor_thermal cooling device"
>> +       depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
>> +       default 20
>> +       help
>> +         The processor_thermal driver uses this config to calculate the
>> +         percentage amount by which cpu frequency must be reduced for each
>> +         cooling state. The config is also used to calculate the maximum number
>> +         of throttling steps or cooling states supported by the driver. Value
>> +         must be an unsigned integer in the range [1, 50].
>> +
> 
> I don't think that the new Kconfig symbols are particularly useful.
> At least they don't help the distro vendors that each would need to
> pick up a specific value for their kernel anyway.
> 
> I also wonder how the users building their own kernels are supposed to
> determine the values suitable for the target systems.
> 

We observed some perf gain after reducing the throttle percentage.
Currently, kept the default to '20%' as before.

Based on need, a vendor can overwrite the default value with macro
'CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG'. Otherwise, the behavior
will remain same.

Thank you,
Sumit Gupta

>>   config ACPI_PROCESSOR_CSTATE
>>          def_bool y
>>          depends on ACPI_PROCESSOR
>> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
>> index b7c6287eccca..ee443cc69b73 100644
>> --- a/drivers/acpi/processor_thermal.c
>> +++ b/drivers/acpi/processor_thermal.c
>> @@ -25,8 +25,20 @@
>>    * _any_ cpufreq driver and not only the acpi-cpufreq driver.
>>    */
>>
>> -#define CPUFREQ_THERMAL_MIN_STEP 0
>> -#define CPUFREQ_THERMAL_MAX_STEP 3
>> +#define CPUFREQ_THERMAL_MIN_STEP       0
>> +#ifdef CONFIG_ACPI_CPU_FREQ_THERM_HAS_PARAMS
>> +#define CPUFREQ_THERMAL_PCTG           CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
>> +
>> +/* Derive the MAX_STEP from minimum throttle percentage so that the reduction
>> + * percentage does end up becoming negative. Also cap the MAX_STEP so that
>> + * the CPU performance doesn't become 0.
>> + */
>> +#define CPUFREQ_THERMAL_MAX_STEP       ((100 / CPUFREQ_THERMAL_PCTG) - 1)
>> +
>> +#else
>> +#define CPUFREQ_THERMAL_MAX_STEP       3
>> +#define CPUFREQ_THERMAL_PCTG           20
>> +#endif
>>
>>   static DEFINE_PER_CPU(unsigned int, cpufreq_thermal_reduction_pctg);
>>
>> @@ -113,7 +125,8 @@ static int cpufreq_set_cur_state(unsigned int cpu, int state)
>>                  if (!policy)
>>                          return -EINVAL;
>>
>> -               max_freq = (policy->cpuinfo.max_freq * (100 - reduction_pctg(i) * 20)) / 100;
>> +               max_freq = (policy->cpuinfo.max_freq *
>> +                           (100 - reduction_pctg(i) * CPUFREQ_THERMAL_PCTG)) / 100;
>>
>>                  cpufreq_cpu_put(policy);
>>
>> --

Re: [Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Rafael J. Wysocki]

On Mon, Aug 21, 2023 at 3:24 PM Sumit Gupta <sumitg@nvidia.com> wrote:
>
>
>
> On 19/08/23 00:10, Rafael J. Wysocki wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, Aug 17, 2023 at 11:31 AM Sumit Gupta <sumitg@nvidia.com> wrote:
> >>
> >> From: Srikar Srimath Tirumala <srikars@nvidia.com>
> >>
> >> Add support to configure the CPUFREQ reduction percentage and set the
> >> maximum number of throttling steps accordingly. Current implementation
> >> of processor_thermal performs software throttling in fixed steps of
> >> "20%" which can be too coarse for some platforms. Change that by adding
> >> new config to provide the reduction percentage.
> >>
> >> Signed-off-by: Srikar Srimath Tirumala <srikars@nvidia.com>
> >> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> >> ---
> >>   drivers/acpi/Kconfig             | 15 +++++++++++++++
> >>   drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
> >>   2 files changed, 31 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 00dd309b6682..287cf58defbf 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -254,6 +254,21 @@ config ACPI_DOCK
> >>   config ACPI_CPU_FREQ_PSS
> >>          bool
> >>
> >> +config ACPI_CPU_FREQ_THERM_HAS_PARAMS
> >> +       bool "CPU frequency throttling control"
> >> +       depends on ACPI_PROCESSOR
> >> +
> >> +config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
> >> +       int "Minimum throttle percentage for processor_thermal cooling device"
> >> +       depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
> >> +       default 20
> >> +       help
> >> +         The processor_thermal driver uses this config to calculate the
> >> +         percentage amount by which cpu frequency must be reduced for each
> >> +         cooling state. The config is also used to calculate the maximum number
> >> +         of throttling steps or cooling states supported by the driver. Value
> >> +         must be an unsigned integer in the range [1, 50].
> >> +
> >
> > I don't think that the new Kconfig symbols are particularly useful.
> > At least they don't help the distro vendors that each would need to
> > pick up a specific value for their kernel anyway.
> >
> > I also wonder how the users building their own kernels are supposed to
> > determine the values suitable for the target systems.
> >
>
> We observed some perf gain after reducing the throttle percentage.
> Currently, kept the default to '20%' as before.

So you should add this information to the patch changelog, ideally
along with the description of the hardware configuration in which the
improvement has been observed.

> Based on need, a vendor can overwrite the default value with macro
> 'CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG'. Otherwise, the behavior
> will remain same.

Yes, that's how it works.

What I'm saying is that the way it works does not appear to be
particularly useful.

For example, how exactly is a distribution supposed to guess the
"right" value for their general-purpose kernel?

Re: [Patch 2/2] ACPI: processor: Add support to configure CPUFREQ reduction pctg

[Sumit Gupta]

>>>>
>>>> Add support to configure the CPUFREQ reduction percentage and set the
>>>> maximum number of throttling steps accordingly. Current implementation
>>>> of processor_thermal performs software throttling in fixed steps of
>>>> "20%" which can be too coarse for some platforms. Change that by adding
>>>> new config to provide the reduction percentage.
>>>>
>>>> Signed-off-by: Srikar Srimath Tirumala <srikars@nvidia.com>
>>>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>>>> ---
>>>>    drivers/acpi/Kconfig             | 15 +++++++++++++++
>>>>    drivers/acpi/processor_thermal.c | 19 ++++++++++++++++---
>>>>    2 files changed, 31 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>>> index 00dd309b6682..287cf58defbf 100644
>>>> --- a/drivers/acpi/Kconfig
>>>> +++ b/drivers/acpi/Kconfig
>>>> @@ -254,6 +254,21 @@ config ACPI_DOCK
>>>>    config ACPI_CPU_FREQ_PSS
>>>>           bool
>>>>
>>>> +config ACPI_CPU_FREQ_THERM_HAS_PARAMS
>>>> +       bool "CPU frequency throttling control"
>>>> +       depends on ACPI_PROCESSOR
>>>> +
>>>> +config ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG
>>>> +       int "Minimum throttle percentage for processor_thermal cooling device"
>>>> +       depends on ACPI_CPU_FREQ_THERM_HAS_PARAMS
>>>> +       default 20
>>>> +       help
>>>> +         The processor_thermal driver uses this config to calculate the
>>>> +         percentage amount by which cpu frequency must be reduced for each
>>>> +         cooling state. The config is also used to calculate the maximum number
>>>> +         of throttling steps or cooling states supported by the driver. Value
>>>> +         must be an unsigned integer in the range [1, 50].
>>>> +
>>>
>>> I don't think that the new Kconfig symbols are particularly useful.
>>> At least they don't help the distro vendors that each would need to
>>> pick up a specific value for their kernel anyway.
>>>
>>> I also wonder how the users building their own kernels are supposed to
>>> determine the values suitable for the target systems.
>>>
>>
>> We observed some perf gain after reducing the throttle percentage.
>> Currently, kept the default to '20%' as before.
> 
> So you should add this information to the patch changelog, ideally
> along with the description of the hardware configuration in which the
> improvement has been observed.
> 

Sure, will add in v2.

>> Based on need, a vendor can overwrite the default value with macro
>> 'CONFIG_ACPI_CPU_FREQ_THERM_MIN_THROT_PCTG'. Otherwise, the behavior
>> will remain same.
> 
> Yes, that's how it works.
> 
> What I'm saying is that the way it works does not appear to be
> particularly useful.
> 
> For example, how exactly is a distribution supposed to guess the
> "right" value for their general-purpose kernel?

We tested on Tegra241 (Grace) SoC with "5%" throttle percentage.
Didn't change the default value as behavior could be different on other 
chips.
An alternate way could be to overwrite the default value for the 
specific SoC using "arm_smccc_get_soc_id_version()" check. But not sure 
if such change in the generic "processor_thermal" driver is Okay?
Please suggest if the above sounds fine or any better way?

Thank you,
Sumit Gupta