Re: [Linaro-acpi] [PATCH linaro-acpi] [RFC] cpufreq: Add ACPI cpufreq support for ARM

Hi Jonghwan,

On 14/04/14 06:53, Jonghwan Choi wrote:

Hi all,

I would like to support acpi-cpufreq for ARM. For that, Firstly, I made new file which is a apci-freq-arm.c. But some people, such as Rafael, Sudeep and Hanjun Guo, worried about that. What ther are worried about is there are too many duplicate code between acpi-cpufreq.c and acpi-cpifre-arm.c So, I tried to separate this code into 3 parts which are common, x86-specific and arm-specific code. I tried to separate soc-specific code from acpi-cpufreq.c as much as possible. But as you know, there are too much x86-specific code in acpi-cpufreq.c When I tried to implement acpi-cpufreq-arm, there were a lot of compile error(due to x86-specific code). So I used #ifdef CONFIG_X86 to solve those errors.(and some hack codes) In this patch, I mostly focused on cpufreq. Later I will remove #ifdef CONFIG_X86. I would like your reviews and comments.

This patch is based on "http://git.linaro.org/leg/acpi/leg-kernel.git"

Last commit : commit a493444ce7f1792b44897160454149dc31ca208b Author: Graeme Gregory graeme.gregory@linaro.org Date: Tue Feb 11 09:21:17 2014 +0000 linaro-configs: add enterprise-distro.conf Signed-off-by: Graeme Gregory graeme.gregory@linaro.org

Thanks Best Regards

Signed-off-by: Jonghwan Choi jhbird.choi@samsung.com

arch/arm64/Kconfig | 12 ++ arch/arm64/include/asm/cpu.h | 3 +- arch/arm64/include/asm/processor.h | 2 + arch/arm64/kernel/process.c | 6 + arch/arm64/kernel/setup.c | 5 +- drivers/acpi/Kconfig | 3 +- drivers/acpi/acpi_processor.c | 4 + drivers/acpi/processor_idle.c | 12 ++ drivers/cpufreq/Kconfig | 2 +- drivers/cpufreq/Kconfig.arm | 16 ++ drivers/cpufreq/Makefile | 3 +- drivers/cpufreq/acpi-cpufreq.c | 419 +++++------------------------------- drivers/cpufreq/acpi-cpufreq.h | 68 ++++++ drivers/cpufreq/acpi-cpufreq_arm.c | 88 ++++++++ drivers/cpufreq/acpi-cpufreq_x86.c | 334 ++++++++++++++++++++++++++++ include/linux/acpi.h | 1 + 16 files changed, 604 insertions(+), 373 deletions(-) create mode 100644 drivers/cpufreq/acpi-cpufreq.h create mode 100644 drivers/cpufreq/acpi-cpufreq_arm.c create mode 100644 drivers/cpufreq/acpi-cpufreq_x86.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 94cc542..e150e60 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -108,6 +108,13 @@ config IOMMU_HELPER config KERNEL_MODE_NEON def_bool y

+config ARCH_HAS_CPUFREQ

•   bool
  

•   help
  

•     Internal node to signify that the ARCH has CPUFREQ support
  

•     and that the relevant menu configurations are displayed for
  

•     it.
  

This is already in the mainline now, not required anymore.

source "init/Kconfig"

source "kernel/Kconfig.freezer" @@ -116,6 +123,7 @@ menu "Platform selection"

config ARCH_VEXPRESS bool "ARMv8 software model (Versatile Express)"

•   select ARCH_HAS_CPUFREQ
  

This makes no sense, the fast models don't support DVFS.

    select ARCH_REQUIRE_GPIOLIB
    select COMMON_CLK_VERSATILE
    select POWER_RESET_VEXPRESS

@@ -325,6 +333,10 @@ endmenu

menu "CPU Power Management"

+if ARCH_HAS_CPUFREQ +source "drivers/cpufreq/Kconfig" +endif

source "drivers/cpuidle/Kconfig"

endmenu diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h index 8625eb1..69db3e7 100644 --- a/arch/arm64/include/asm/cpu.h +++ b/arch/arm64/include/asm/cpu.h @@ -20,6 +20,7 @@ struct cpuinfo_arm { #endif };

-DECLARE_PER_CPU(struct cpuinfo_arm, cpu_data); +DECLARE_PER_CPU(struct cpuinfo_arm, cpu_info); +#define cpu_data(cpu) per_cpu(cpu_info, cpu)

This looks like unnecessary change, as you don't use this.

#endif diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 50ce951..7196873 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -47,6 +47,8 @@ #define ARCH_LOW_ADDRESS_LIMIT PHYS_MASK #endif /* __KERNEL__ */

+enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT,

•                    IDLE_POLL};
  

This doesn't belong to cpufreq related changes

struct debug_info { /* Have we suspended stepping by a debugger? */ int suspended_step; diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 1c0a9be..1c985a9 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -86,6 +86,12 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); EXPORT_SYMBOL_GPL(arm_pm_restart);

/*

• • Idle related variables and functions */ unsigned long

+boot_option_idle_override = IDLE_NO_OVERRIDE; +EXPORT_SYMBOL(boot_option_idle_override);

+/*

Again this doesn't belong to cpufreq related changes.

• This is our default idle handler.

*/ void arch_cpu_idle(void) diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index fecf272..99b973b 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -377,14 +377,15 @@ static int __init arm64_device_init(void) } arch_initcall(arm64_device_init);

-static DEFINE_PER_CPU(struct cpu, cpu_data); +DEFINE_PER_CPU(struct cpu, cpu_info); +EXPORT_PER_CPU_SYMBOL(cpu_info);

Now I think this is introduced just to fix some compilation ?

static int __init topology_init(void) { int i;

    for_each_possible_cpu(i) {

•           struct cpu *cpu = &per_cpu(cpu_data, i);
  

•           struct cpu *cpu = &per_cpu(cpu_info, i);
  

This is want I meant above, you define a macro for this and not use it here ?

            cpu->hotpluggable = 1;
            register_cpu(cpu, i);
    }

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index cbc5dfc..961211f 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -152,7 +152,7 @@ config ACPI_PROCESSOR tristate "Processor" select THERMAL select CPU_IDLE

•   depends on X86 || IA64
  

•   depends on X86 || IA64 || ARM64
    default y
    help
      This driver installs ACPI as the idle handler for Linux and uses
  

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index c29c2c3..6a89856 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -177,6 +177,7 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr) cpu_maps_update_begin(); cpu_ho\12.36.155.51\jhbirdchoi\Kernel\leg-kerneltplug_begin();

???

+#ifdef CONFIG_X86 ret = acpi_map_lsapic(pr->handle, pr->apic_id, &pr->id); if (ret) goto out; @@ -186,6 +187,7 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr) acpi_unmap_lsapic(pr->id); goto out; } +#endif

    /*
     * CPU got hot-added, but cpu_data is not initialized yet.  Set a

flag @@ -457,8 +459,10 @@ static void acpi_processor_remove(struct acpi_device *device) cpu_hotplug_begin();

    /* Remove the CPU. */

+#ifdef CONFIG_X86 arch_unregister_cpu(pr->id);

Why is this X86 specific ?

    acpi_unmap_lsapic(pr->id);

+#endif

    cpu_hotplug_done();
    cpu_maps_update_done();

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 3dca36d..0556a0a 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c

These changes in this file doesn't belong to cpufreq related changes. IMO Moving all such hack separately to another patch makes it easy to review.

[...]

%d\n", diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index 3a7202d..6417406 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -228,7 +228,7 @@ source "drivers/cpufreq/Kconfig.x86" endmenu

menu "ARM CPU frequency scaling drivers" -depends on ARM +depends on ARM64

Interesting why are you removing ARM ?

source "drivers/cpufreq/Kconfig.arm" endmenu

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 3129749..ef00a4c 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -16,6 +16,21 @@ config ARM_DT_BL_CPUFREQ This enables probing via DT for Generic CPUfreq driver for ARM big.LITTLE platform. This gets frequency tables from DT.

+config ARM_ACPI_CPUFREQ

•   tristate "ACPI Processor P-States driver"
  

•   depends on ACPI_PROCESSOR
  

•   help
  

•     This driver adds a CPUFreq driver which utilizes the ACPI
  

•     Processor Performance States.
  

•     This driver also supports ARM CPUs.
  

•     To compile this driver as a module, choose M here: the
  

•     module will be called acpi-cpufreq.
  

•     For details, take a look at <file:Documentation/cpu-freq/>.
  

•     If in doubt, say N.
  

Do we really need this if we must have single ACPI CPUFreq driver ?

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 18448a7..26bf7112 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -42,37 +42,17 @@

#include <acpi/processor.h>

-#include <asm/msr.h> +#include <asm/cpu.h> #include <asm/processor.h> -#include <asm/cpufeature.h>

+#include "acpi-cpufreq.h"

I am skipping all x86 changes for now.

diff --git a/drivers/cpufreq/acpi-cpufreq_arm.c b/drivers/cpufreq/acpi-cpufreq_arm.c new file mode 100644 index 0000000..241c2a1 --- /dev/null +++ b/drivers/cpufreq/acpi-cpufreq_arm.c @@ -0,0 +1,88 @@ +#include <linux/acpi.h> +#include <linux/cpu.h> +#include <linux/io.h> +#include <linux/delay.h> +#include <linux/uaccess.h>

+#include <acpi/processor.h>

+#include <asm/cpu.h> +#include <asm/processor.h>

+#include "acpi-cpufreq.h"

+/* Called via smp_call_function_single(), on the target CPU */ static +void arm_do_drv_read(void *_cmd, struct acpi_cpufreq_data *data) {

•   struct drv_cmd *cmd = _cmd;
  

•   if (data->cpu_feature != SYSTEM_MEMORY_CAPABLE)
  

•           return;
  

This is the core part I would like to understand. So far very few(close to none) ARM SoCs have such memory mapped interface for DVFS. Is this based on you assumption ? Or have you come across such SoC/platforms ?

Even if this is present, we need to think of catering other possible alternate methods we might have to support on ARM SoCs.

•   acpi_os_read_memory((acpi_physical_address)cmd->addr.mem.addr,
  

•                   (u64*)&cmd->val,
  

•                   cmd->addr.mem.bit_width);
  

Further all you are using is standard System Memory as the Address Space ID, hence IMO this can be part of generic acpi cpufreq driver. I don't see anything ARM specific in Address Space ID being System Memory.

Regards, Sudeep