| Message ID | 20200202230626.6598-1-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers/firmware/psci: Hide ACPI state during initialization | expand |
On Mon, Feb 03, 2020 at 10:06:26AM +1100, Gavin Shan wrote: > Either psci_dt_init() or psci_acpi_init() s called depends on ACPI > enablement state, which isn't so nice. In this case, two functions > have to be exported from PSCI module. > I am confused, we don't export any functions as you mention and both are __init functions which can't be exported. > This hides the ACPI enablement state insides PSCI module so that we > only need to export a function, to make the code a bit simplified. > For me it's just the preference. I will leave it to maintainers' taste. -- Regards, Sudeep
Hi Sudeep, On Mon, Feb. 3, 2020, 11:45 a.m. UTC, Sudeep Holla wrote: > On Mon, Feb 03, 2020 at 10:06:26AM +1100, Gavin Shan wrote: >> Either psci_dt_init() or psci_acpi_init() s called depends on ACPI >> enablement state, which isn't so nice. In this case, two functions >> have to be exported from PSCI module. >> I missed your reply and sorry for late catchup. > I am confused, we don't export any functions as you mention and both > are __init functions which can't be exported. > The words "export" here means "declared". Two functons (psci_{dt,acpi}_init()) are declared and one of them is called depending on ACPI is enabled or not. If we hide the ACPI enablement state inside the driver/module, we just need to declare one function (psci_init()), to make the code a bit cleaner. >> This hides the ACPI enablement state insides PSCI module so that we >> only need to export a function, to make the code a bit simplified. >> > For me it's just the preference. I will leave it to maintainers' taste. > Sure, lets see what comments Mark will have then. It's not bad to make the code cleaner, even just a bit :) Thank you for your time on this. Thanks, Gavin
On Tue, Feb 11, 2020 at 01:02:44PM +1100, Gavin Shan wrote: [...] > The words "export" here means "declared". Two functons (psci_{dt,acpi}_init()) > are declared and one of them is called depending on ACPI is enabled or not. If > we hide the ACPI enablement state inside the driver/module, we just need to > declare one function (psci_init()), to make the code a bit cleaner. > > > > This hides the ACPI enablement state insides PSCI module so that we > > > only need to export a function, to make the code a bit simplified. > > > > > > For me it's just the preference. I will leave it to maintainers' taste. I am not too fussed either way. As code is now though at least we know acpi_disabled was {set/clear} before PSCI is initialized. Hiding the ACPI/DT switch in PSCI code can be a problem if we move the boot code around. I don't necessarily see this patch as an improvement, again it is no big deal regardless. Thanks, Lorenzo
On 2/11/20 11:21 PM, Lorenzo Pieralisi wrote: > On Tue, Feb 11, 2020 at 01:02:44PM +1100, Gavin Shan wrote: > > [...] > >> The words "export" here means "declared". Two functons (psci_{dt,acpi}_init()) >> are declared and one of them is called depending on ACPI is enabled or not. If >> we hide the ACPI enablement state inside the driver/module, we just need to >> declare one function (psci_init()), to make the code a bit cleaner. >> >>>> This hides the ACPI enablement state insides PSCI module so that we >>>> only need to export a function, to make the code a bit simplified. >>>> >> >>> For me it's just the preference. I will leave it to maintainers' taste. > I am not too fussed either way. As code is now though at least we know > acpi_disabled was {set/clear} before PSCI is initialized. Hiding the > ACPI/DT switch in PSCI code can be a problem if we move the boot code > around. > > I don't necessarily see this patch as an improvement, again it is > no big deal regardless. > Lorenzo, thanks a lot for the explanation. I'm fine with either way. Please pick it if it's fine to you. Otherwise, please drop this :) Thanks, Gavin
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index a34890bf309f..9fce20bec720 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -338,12 +338,7 @@ void __init setup_arch(char **cmdline_p) request_standard_resources(); early_ioremap_reset(); - - if (acpi_disabled) - psci_dt_init(); - else - psci_acpi_init(); - + psci_init(); cpu_read_bootcpu_ops(); smp_init_cpus(); smp_build_mpidr_hash(); diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index b3b6c15e7b36..f2f5db35d896 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -557,7 +557,7 @@ static const struct of_device_id psci_of_match[] __initconst = { {}, }; -int __init psci_dt_init(void) +static int __init psci_dt_init(void) { struct device_node *np; const struct of_device_id *matched_np; @@ -581,7 +581,7 @@ int __init psci_dt_init(void) * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's * explicitly clarified in SBBR */ -int __init psci_acpi_init(void) +static int __init psci_acpi_init(void) { if (!acpi_psci_present()) { pr_info("is not implemented in ACPI.\n"); @@ -597,4 +597,18 @@ int __init psci_acpi_init(void) return psci_probe(); } -#endif + +#else +static inline int psci_acpi_init(void) +{ + return 0; +} +#endif /* CONFIG_ACPI */ + +int __init psci_init(void) +{ + if (acpi_disabled) + return psci_dt_init(); + + return psci_acpi_init(); +} diff --git a/include/linux/psci.h b/include/linux/psci.h index ebe0a881d13d..64af884a8d96 100644 --- a/include/linux/psci.h +++ b/include/linux/psci.h @@ -39,18 +39,16 @@ struct psci_operations { extern struct psci_operations psci_ops; -#if defined(CONFIG_ARM_PSCI_FW) -int __init psci_dt_init(void); +#ifdef CONFIG_ARM_PSCI_FW +int __init psci_init(void); #else -static inline int psci_dt_init(void) { return 0; } +static inline int psci_init(void) { return 0; } #endif #if defined(CONFIG_ARM_PSCI_FW) && defined(CONFIG_ACPI) -int __init psci_acpi_init(void); bool __init acpi_psci_present(void); bool acpi_psci_use_hvc(void); #else -static inline int psci_acpi_init(void) { return 0; } static inline bool acpi_psci_present(void) { return false; } static inline bool acpi_psci_use_hvc(void) {return false; } #endif
Either psci_dt_init() or psci_acpi_init() s called depends on ACPI enablement state, which isn't so nice. In this case, two functions have to be exported from PSCI module. This hides the ACPI enablement state insides PSCI module so that we only need to export a function, to make the code a bit simplified. Signed-off-by: Gavin Shan <gshan@redhat.com> --- arch/arm64/kernel/setup.c | 7 +------ drivers/firmware/psci/psci.c | 20 +++++++++++++++++--- include/linux/psci.h | 8 +++----- 3 files changed, 21 insertions(+), 14 deletions(-)