Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Hanjun Guo <hanjun.guo@linaro.org>]

On 2014年01月23日 23:56, Tomasz Nowicki wrote:
> Hi Lorenzo,
>
> W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:
>> On Fri, Jan 17, 2014 at 12:24:58PM +0000, Hanjun Guo wrote:
>>
>> [...]
>>
>>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>>> index bd9bbd0..2210353 100644
>>> --- a/arch/arm64/kernel/setup.c
>>> +++ b/arch/arm64/kernel/setup.c
>>> @@ -41,6 +41,7 @@
>>> #include <linux/memblock.h>
>>> #include <linux/of_fdt.h>
>>> #include <linux/of_platform.h>
>>> +#include <linux/acpi.h>
>>>
>>> #include <asm/cputype.h>
>>> #include <asm/elf.h>
>>> @@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)
>>>
>>> arm64_memblock_init();
>>>
>>> + /* Parse the ACPI tables for possible boot-time configuration */
>>> + acpi_boot_table_init();
>>> + early_acpi_boot_init();
>>> + acpi_boot_init();
>>> +
>>> paging_init();
>>
>> Can I ask you please why we need to parse ACPI tables before
>> paging_init() ?
> This is for future usage and because of couple of reasons. Mainly SRAT 
> table parsing should be done (before paging_init()) for proper NUMA 
> initialization and then paging_init().

Yes, I agree, thanks for Tomasz's clarification.

>>
>> [...]
>>
>>> +/*
>>> + * __acpi_map_table() will be called before page_init(), so 
>>> early_ioremap()
>>> + * or early_memremap() should be called here.
>>
>> Again, why is this needed ? What's needed before paging_init() from 
>> ACPI ?
>>
>> [...]
>>
>>> +/*
>>> + * acpi_boot_table_init() and acpi_boot_init()
>>> + * called from setup_arch(), always.
>>> + * 1. checksums all tables
>>> + * 2. enumerates lapics
>>> + * 3. enumerates io-apics
>>> + *
>>> + * acpi_table_init() is separated to allow reading SRAT without
>>> + * other side effects.
>>> + */
>>> +void __init acpi_boot_table_init(void)
>>> +{
>>> + /*
>>> + * If acpi_disabled, bail out
>>> + */
>>> + if (acpi_disabled)
>>> + return;
>>> +
>>> + /*
>>> + * Initialize the ACPI boot-time table parser.
>>> + */
>>> + if (acpi_table_init()) {
>>> + disable_acpi();
>>> + return;
>>> + }
>>> +}
>>> +
>>> +int __init early_acpi_boot_init(void)
>>> +{
>>> + /*
>>> + * If acpi_disabled, bail out
>>> + */
>>> + if (acpi_disabled)
>>> + return -ENODEV;
>>> +
>>> + /*
>>> + * Process the Multiple APIC Description Table (MADT), if present
>>> + */
>>> + early_acpi_process_madt();
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int __init acpi_boot_init(void)
>>> +{
>>> + /*
>>> + * If acpi_disabled, bail out
>>> + */
>>> + if (acpi_disabled)
>>> + return -ENODEV;
>>> +
>>> + acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
>>> +
>>> + /*
>>> + * Process the Multiple APIC Description Table (MADT), if present
>>> + */
>>> + acpi_process_madt();
>>> +
>>> + return 0;
>>> +}
>>
>> Well, apart from having three init calls, one returning void and two
>> returning proper values, do not understand why, and do not understand
>> why we need three calls in the first place...why should we process MADT
>> twice in two separate calls ? What is supposed to change in between that
>> prevents you from merging the two together ?

Thanks for pointing this out. I can merge acpi_boot_table_init() and
early_acpi_boot_init() together, but can not merge early_acpi_boot_init()
and acpi_boot_init() together.

early_acpi_boot_init() and acpi_boot_init() was separated intentionally for
memory hotplug reasons. memory allocated in this stage can not be migrated
and cause memory hot-remove failed, in order to keep memory allocated
at base node (general NUMA node 0 in the system) at boot stage, we should
parse SRAT first before CPU is enumerated, does this make sense to you?

Thanks
Hanjun