| Message ID | 1422881149-8177-12-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2015年02月05日 00:43, Lorenzo Pieralisi wrote: > On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote: >> From: Graeme Gregory <graeme.gregory@linaro.org> >> >> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, >> the former signals to the OS that the firmware is PSCI compliant. >> The latter selects the appropriate conduit for PSCI calls by >> toggling between Hypervisor Calls (HVC) and Secure Monitor Calls >> (SMC). >> >> FADT table contains such information in ACPI 5.1, FADT table was >> parsed in ACPI table init and copy to struct acpi_gbl_FADT, so >> use the flags in struct acpi_gbl_FADT for PSCI init. > > So you do rely on a global FADT being available, if you use it for PSCI > detection you can use it for ACPI revision detection too, right ? Yes, I think so. > > Point is, either we should not use the global FADT table, or we use > it consistently, or there is something I am unaware of that prevents > you from using in some code paths and I would like to understand > why. global FADT table is initialized when parsing the tables from RSDP in ACPICA core, and it should be work on ARM64 too. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/04/2015 09:43 AM, Lorenzo Pieralisi wrote: > On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote: >> From: Graeme Gregory <graeme.gregory@linaro.org> >> >> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, >> the former signals to the OS that the firmware is PSCI compliant. >> The latter selects the appropriate conduit for PSCI calls by >> toggling between Hypervisor Calls (HVC) and Secure Monitor Calls >> (SMC). >> >> FADT table contains such information in ACPI 5.1, FADT table was >> parsed in ACPI table init and copy to struct acpi_gbl_FADT, so >> use the flags in struct acpi_gbl_FADT for PSCI init. > > So you do rely on a global FADT being available, if you use it for PSCI > detection you can use it for ACPI revision detection too, right ? > > Point is, either we should not use the global FADT table, or we use > it consistently, or there is something I am unaware of that prevents > you from using in some code paths and I would like to understand > why. The FADT is a required table for arm64, as noted in the documentation and the SBBR. While unfortunately the spec does not say it is mandatory, even x86 systems are pretty useless without it. So yes, we rely on it being available, not only for the PSCI info, but other flags such as HW_REDUCED_ACPI. I suppose it does not have to be globally scoped. However, the FADT is frequently used, especially on x86, so it makes sense to me from an efficiency standpoint to have a global reference to it. I'm not sure I understand what is meant by using FADT for ACPI revision detection; there are fields in the FADT that provide a major and minor number for the FADT itself, but I don't believe there's any guarantee those will be the same as the level of the specification that is being supported by the kernel (chances are they will, but it's not mandatory). I've probably just missed a part of a thread somewhere; could you point me to where the inconsistency lies? I'm just not understanding right this second....
On 02/05/2015 10:49 AM, Lorenzo Pieralisi wrote: > Hi Al, Howdy, Lorenzo. > On Thu, Feb 05, 2015 at 05:11:31PM +0000, Al Stone wrote: >> On 02/04/2015 09:43 AM, Lorenzo Pieralisi wrote: >>> On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote: >>>> From: Graeme Gregory <graeme.gregory@linaro.org> >>>> >>>> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, >>>> the former signals to the OS that the firmware is PSCI compliant. >>>> The latter selects the appropriate conduit for PSCI calls by >>>> toggling between Hypervisor Calls (HVC) and Secure Monitor Calls >>>> (SMC). >>>> >>>> FADT table contains such information in ACPI 5.1, FADT table was >>>> parsed in ACPI table init and copy to struct acpi_gbl_FADT, so >>>> use the flags in struct acpi_gbl_FADT for PSCI init. >>> >>> So you do rely on a global FADT being available, if you use it for PSCI >>> detection you can use it for ACPI revision detection too, right ? >>> >>> Point is, either we should not use the global FADT table, or we use >>> it consistently, or there is something I am unaware of that prevents >>> you from using in some code paths and I would like to understand >>> why. >> >> The FADT is a required table for arm64, as noted in the documentation >> and the SBBR. While unfortunately the spec does not say it is mandatory, >> even x86 systems are pretty useless without it. So yes, we rely on it >> being available, not only for the PSCI info, but other flags such as >> HW_REDUCED_ACPI. >> >> I suppose it does not have to be globally scoped. However, the FADT is >> frequently used, especially on x86, so it makes sense to me from an >> efficiency standpoint to have a global reference to it. >> >> I'm not sure I understand what is meant by using FADT for ACPI revision >> detection; there are fields in the FADT that provide a major and minor >> number for the FADT itself, but I don't believe there's any guarantee >> those will be the same as the level of the specification that is being >> supported by the kernel (chances are they will, but it's not mandatory). >> >> I've probably just missed a part of a thread somewhere; could you point >> me to where the inconsistency lies? I'm just not understanding right this >> second.... > > Yes, it is my fault, I was referring to another thread/patch (9), where you > need to check the FADT revision to "validate it" (ie >= 5.1) for the arm64 > kernel. What I am saying is: if the global FADT is there to parse PSCI > info, it is there to check the FADT revision too, I do not necessarily > see the need for calling acpi_table_parse() again to do it, the FADT > revision checking can be carried out as for PSCI, that's all I wanted > to say. > > Thanks, > Lorenzo > Aha. I understand now. Another colleague was also trying to explain this to me and I think I just hadn't had enough coffee yet. The underlying ACPI code maps tables into the kernel in two phases; it may be that when the code in patch 9 is run, the global table is not yet available, while here it is; I don't recall off-hand. I'll take a look at this and talk it over with Hanjun. If the global table is available, it would indeed make sense to be consistent. Thanks for the explanation; that really helped me.
Hi Lorenzo, Al, On 2015年02月06日 03:03, Al Stone wrote: > On 02/05/2015 10:49 AM, Lorenzo Pieralisi wrote: >> Hi Al, > > Howdy, Lorenzo. > >> On Thu, Feb 05, 2015 at 05:11:31PM +0000, Al Stone wrote: >>> On 02/04/2015 09:43 AM, Lorenzo Pieralisi wrote: >>>> On Mon, Feb 02, 2015 at 12:45:39PM +0000, Hanjun Guo wrote: >>>>> From: Graeme Gregory <graeme.gregory@linaro.org> >>>>> >>>>> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, >>>>> the former signals to the OS that the firmware is PSCI compliant. >>>>> The latter selects the appropriate conduit for PSCI calls by >>>>> toggling between Hypervisor Calls (HVC) and Secure Monitor Calls >>>>> (SMC). >>>>> >>>>> FADT table contains such information in ACPI 5.1, FADT table was >>>>> parsed in ACPI table init and copy to struct acpi_gbl_FADT, so >>>>> use the flags in struct acpi_gbl_FADT for PSCI init. >>>> >>>> So you do rely on a global FADT being available, if you use it for PSCI >>>> detection you can use it for ACPI revision detection too, right ? >>>> >>>> Point is, either we should not use the global FADT table, or we use >>>> it consistently, or there is something I am unaware of that prevents >>>> you from using in some code paths and I would like to understand >>>> why. >>> >>> The FADT is a required table for arm64, as noted in the documentation >>> and the SBBR. While unfortunately the spec does not say it is mandatory, >>> even x86 systems are pretty useless without it. So yes, we rely on it >>> being available, not only for the PSCI info, but other flags such as >>> HW_REDUCED_ACPI. >>> >>> I suppose it does not have to be globally scoped. However, the FADT is >>> frequently used, especially on x86, so it makes sense to me from an >>> efficiency standpoint to have a global reference to it. >>> >>> I'm not sure I understand what is meant by using FADT for ACPI revision >>> detection; there are fields in the FADT that provide a major and minor >>> number for the FADT itself, but I don't believe there's any guarantee >>> those will be the same as the level of the specification that is being >>> supported by the kernel (chances are they will, but it's not mandatory). >>> >>> I've probably just missed a part of a thread somewhere; could you point >>> me to where the inconsistency lies? I'm just not understanding right this >>> second.... >> >> Yes, it is my fault, I was referring to another thread/patch (9), where you >> need to check the FADT revision to "validate it" (ie >= 5.1) for the arm64 >> kernel. What I am saying is: if the global FADT is there to parse PSCI >> info, it is there to check the FADT revision too, I do not necessarily >> see the need for calling acpi_table_parse() again to do it, the FADT >> revision checking can be carried out as for PSCI, that's all I wanted >> to say. > > Aha. I understand now. Another colleague was also trying to explain this > to me and I think I just hadn't had enough coffee yet. The underlying ACPI > code maps tables into the kernel in two phases; it may be that when the code > in patch 9 is run, the global table is not yet available, while here it is; > I don't recall off-hand. > > I'll take a look at this and talk it over with Hanjun. If the global table > is available, it would indeed make sense to be consistent. I had dig into the code and found out that struct acpi_gbl_FADT will be available with correct value only if FADT is presented by firmware. acpi_table_init() will be called before parsing FADT for PSCI flag in this patch set. In acpi_table_init() acpi_initialize_tables() acpi_tb_parse_root_table() In acpi_tb_parse_root_table() if (ACPI_SUCCESS(status) && ACPI_COMPARE_NAME(&acpi_gbl_root_table_list. tables[table_index].signature, ACPI_SIG_FADT)) { acpi_tb_parse_fadt(table_index); } And acpi_tb_parse_fadt(table_index) will copy the fadt table to global struct acpi_gbl_FADT. so it seems that we can use global struct acpi_gbl_FADT directly to check the FADT revision, but it is only available with firmware presented the FADT table, so check for the FADT table is still needed for some bad firmware without FADT. Why PSCI flag can be used without any check for the availability of FADT? because we already disable ACPI if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) failed (no FADT tabled found), and PSCI flag will not be used later. So I think we can keep the code as it is for now, and I think it is the safest way to do it, does it make sense? Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 9fcf632..1aea87c 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,18 @@ extern int acpi_disabled; extern int acpi_noirq; extern int acpi_pci_disabled; +/* 1 to indicate PSCI 0.2+ is implemented */ +static inline bool acpi_psci_present(void) +{ + return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT; +} + +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */ +static inline bool acpi_psci_use_hvc(void) +{ + return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC; +} + static inline void disable_acpi(void) { acpi_disabled = 1; @@ -50,6 +62,8 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { } #else static inline void disable_acpi(void) { } static inline void enable_acpi(void) { } +static inline bool acpi_psci_present(void) { return false; } +static inline bool acpi_psci_use_hvc(void) { return false; } #endif /* CONFIG_ACPI */ #endif /*_ASM_ACPI_H*/ diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h index e5312ea..2454bc5 100644 --- a/arch/arm64/include/asm/psci.h +++ b/arch/arm64/include/asm/psci.h @@ -14,6 +14,7 @@ #ifndef __ASM_PSCI_H #define __ASM_PSCI_H -int psci_init(void); +int psci_dt_init(void); +int psci_acpi_init(void); #endif /* __ASM_PSCI_H */ diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index f1dbca7..0ec0dc5 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -15,6 +15,7 @@ #define pr_fmt(fmt) "psci: " fmt +#include <linux/acpi.h> #include <linux/init.h> #include <linux/of.h> #include <linux/smp.h> @@ -24,6 +25,7 @@ #include <linux/slab.h> #include <uapi/linux/psci.h> +#include <asm/acpi.h> #include <asm/compiler.h> #include <asm/cpu_ops.h> #include <asm/errno.h> @@ -304,6 +306,33 @@ static void psci_sys_poweroff(void) invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); } +static void __init psci_0_2_set_functions(void) +{ + pr_info("Using standard PSCI v0.2 function IDs\n"); + psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; + psci_ops.cpu_suspend = psci_cpu_suspend; + + psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; + psci_ops.cpu_off = psci_cpu_off; + + psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; + psci_ops.cpu_on = psci_cpu_on; + + psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; + psci_ops.migrate = psci_migrate; + + psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; + psci_ops.affinity_info = psci_affinity_info; + + psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = + PSCI_0_2_FN_MIGRATE_INFO_TYPE; + psci_ops.migrate_info_type = psci_migrate_info_type; + + arm_pm_restart = psci_sys_reset; + + pm_power_off = psci_sys_poweroff; +} + /* * PSCI Function IDs for v0.2+ are well defined so use * standard values. @@ -337,29 +366,7 @@ static int __init psci_0_2_init(struct device_node *np) } } - pr_info("Using standard PSCI v0.2 function IDs\n"); - psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND; - psci_ops.cpu_suspend = psci_cpu_suspend; - - psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF; - psci_ops.cpu_off = psci_cpu_off; - - psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON; - psci_ops.cpu_on = psci_cpu_on; - - psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE; - psci_ops.migrate = psci_migrate; - - psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO; - psci_ops.affinity_info = psci_affinity_info; - - psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] = - PSCI_0_2_FN_MIGRATE_INFO_TYPE; - psci_ops.migrate_info_type = psci_migrate_info_type; - - arm_pm_restart = psci_sys_reset; - - pm_power_off = psci_sys_poweroff; + psci_0_2_set_functions(); out_put_node: of_node_put(np); @@ -412,7 +419,7 @@ static const struct of_device_id psci_of_match[] __initconst = { {}, }; -int __init psci_init(void) +int __init psci_dt_init(void) { struct device_node *np; const struct of_device_id *matched_np; @@ -427,6 +434,29 @@ int __init psci_init(void) return init_fn(np); } +/* + * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's + * explicitly clarified in SBBR + */ +int __init psci_acpi_init(void) +{ + if (!acpi_psci_present()) { + pr_info("is not implemented in ACPI.\n"); + return -EOPNOTSUPP; + } + + pr_info("probing for conduit method from ACPI.\n"); + + if (acpi_psci_use_hvc()) + invoke_psci_fn = __invoke_psci_fn_hvc; + else + invoke_psci_fn = __invoke_psci_fn_smc; + + psci_0_2_set_functions(); + + return 0; +} + #ifdef CONFIG_SMP static int __init cpu_psci_cpu_init(struct device_node *dn, unsigned int cpu) diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 553967d..43ae914 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -446,10 +446,12 @@ void __init setup_arch(char **cmdline_p) efi_idmap_init(); early_ioremap_reset(); - if (acpi_disabled) + if (acpi_disabled) { unflatten_device_tree(); - - psci_init(); + psci_dt_init(); + } else { + psci_acpi_init(); + } cpu_read_bootcpu_ops(); #ifdef CONFIG_SMP