On 19 January 2015 at 11:42, Catalin Marinas catalin.marinas@arm.com wrote:

On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote:

...

From: Al Stone al.stone@linaro.org

Introduce one early parameters "off" and "force" for "acpi", acpi=off will be the default behavior for ARM64, so introduce acpi=force to enable ACPI on ARM64.

Disable ACPI before early parameters parsed, and enable it to pass "acpi=force" if people want use ACPI on ARM64. This ensures DT be the prefer one if ACPI table and DT both are provided at this moment.

[...]

...

--- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -62,6 +62,7 @@ #include <asm/memblock.h> #include <asm/psci.h> #include <asm/efi.h> +#include <asm/acpi.h>

unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init();

• disable_acpi();
  

• parse_early_param();
  
  /*
  

Did we get to any conclusion here? DT being the preferred one is fine when both DT and ACPI are present but do we still want the kernel to ignore ACPI altogether if DT is not present? It's a bit harder to detect the presence of DT at this point since the EFI_STUB added one already. I guess we could move the "acpi=force" argument passing to EFI_STUB if no DT is present at boot.

Since the EFI stub populates the /chosen node in DT, I would prefer for it to add a property there to indicate whether it created the DT from scratch rather than adding ACPI specific stuff in there (even if it is just a string to concatenate)

On 19 January 2015 at 13:51, Catalin Marinas catalin.marinas@arm.com wrote:

On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote:

...

On 19 January 2015 at 11:42, Catalin Marinas catalin.marinas@arm.com wrote:

...

On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote:

...

From: Al Stone al.stone@linaro.org

Introduce one early parameters "off" and "force" for "acpi", acpi=off will be the default behavior for ARM64, so introduce acpi=force to enable ACPI on ARM64.

Disable ACPI before early parameters parsed, and enable it to pass "acpi=force" if people want use ACPI on ARM64. This ensures DT be the prefer one if ACPI table and DT both are provided at this moment.

[...]

...

--- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -62,6 +62,7 @@ #include <asm/memblock.h> #include <asm/psci.h> #include <asm/efi.h> +#include <asm/acpi.h>

unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init();

• disable_acpi();
  

• parse_early_param();
  
  /*
  

Did we get to any conclusion here? DT being the preferred one is fine when both DT and ACPI are present but do we still want the kernel to ignore ACPI altogether if DT is not present? It's a bit harder to detect the presence of DT at this point since the EFI_STUB added one already. I guess we could move the "acpi=force" argument passing to EFI_STUB if no DT is present at boot.

Since the EFI stub populates the /chosen node in DT, I would prefer for it to add a property there to indicate whether it created the DT from scratch rather than adding ACPI specific stuff in there (even if it is just a string to concatenate)

This works for me. So we could pass "acpi=force" in EFI stub if it created the DT from scratch *and* ACPI tables are present (can it detect the latter? And maybe it could print something if none are available). If that works, the actual kernel can assume that ACPI needs to be explicitly enabled via acpi=force, irrespective of how much information it has in DT.

Erm, that is not at all what I meant. What I meant was, that if it is interesting to the kernel proper to know whether the DT was created from scratch by the stub rather than received from the firmware, we can record that particular fact in the /chosen node, and nothing else. How this is interpreted is up to the kernel proper entirely.

Note that the stub may outlive many subsequent kexec reboots, so dividing policy like this across the stub/kernel boundary is asking for trouble imo. For instance, booting with a DT via kexec would be impossible unless we add special handling for this case, which is exactly what I tried to avoid with my latest virtmap series.

On Mon, 19 Jan 2015 13:51:45 +0000 , Catalin Marinas catalin.marinas@arm.com wrote:

On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote:

...

On 19 January 2015 at 11:42, Catalin Marinas catalin.marinas@arm.com wrote:

...

On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote:

...

From: Al Stone al.stone@linaro.org

Introduce one early parameters "off" and "force" for "acpi", acpi=off will be the default behavior for ARM64, so introduce acpi=force to enable ACPI on ARM64.

Disable ACPI before early parameters parsed, and enable it to pass "acpi=force" if people want use ACPI on ARM64. This ensures DT be the prefer one if ACPI table and DT both are provided at this moment.

[...]

...

--- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -62,6 +62,7 @@ #include <asm/memblock.h> #include <asm/psci.h> #include <asm/efi.h> +#include <asm/acpi.h>

unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init();

• disable_acpi();
  

• parse_early_param();
  
  /*
  

Did we get to any conclusion here? DT being the preferred one is fine when both DT and ACPI are present but do we still want the kernel to ignore ACPI altogether if DT is not present? It's a bit harder to detect the presence of DT at this point since the EFI_STUB added one already. I guess we could move the "acpi=force" argument passing to EFI_STUB if no DT is present at boot.

Since the EFI stub populates the /chosen node in DT, I would prefer for it to add a property there to indicate whether it created the DT from scratch rather than adding ACPI specific stuff in there (even if it is just a string to concatenate)

This works for me. So we could pass "acpi=force" in EFI stub if it created the DT from scratch *and* ACPI tables are present (can it detect the latter? And maybe it could print something if none are available). If that works, the actual kernel can assume that ACPI needs to be explicitly enabled via acpi=force, irrespective of how much information it has in DT.

Ditto for me. I think this is a fine solution. And, yes, the stub can easily detect the presence of ACPI by looking in the UEFI config table.

g.

On Mon, Jan 19, 2015 at 04:59:47PM +0000, Jon Masters wrote:

On 01/19/2015 10:13 AM, Grant Likely wrote:

...

On Mon, 19 Jan 2015 13:51:45 +0000 , Catalin Marinas catalin.marinas@arm.com wrote:

...

On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote:

...

On 19 January 2015 at 11:42, Catalin Marinas catalin.marinas@arm.com wrote:

...

On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote:

...

From: Al Stone al.stone@linaro.org

Introduce one early parameters "off" and "force" for "acpi", acpi=off will be the default behavior for ARM64, so introduce acpi=force to enable ACPI on ARM64.

Disable ACPI before early parameters parsed, and enable it to pass "acpi=force" if people want use ACPI on ARM64. This ensures DT be the prefer one if ACPI table and DT both are provided at this moment.

[...]

...

--- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -62,6 +62,7 @@ #include <asm/memblock.h> #include <asm/psci.h> #include <asm/efi.h> +#include <asm/acpi.h>

unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init();

• disable_acpi();
  

• parse_early_param();
  
  /*
  

Did we get to any conclusion here? DT being the preferred one is fine when both DT and ACPI are present but do we still want the kernel to ignore ACPI altogether if DT is not present? It's a bit harder to detect the presence of DT at this point since the EFI_STUB added one already. I guess we could move the "acpi=force" argument passing to EFI_STUB if no DT is present at boot.

Since the EFI stub populates the /chosen node in DT, I would prefer for it to add a property there to indicate whether it created the DT from scratch rather than adding ACPI specific stuff in there (even if it is just a string to concatenate)

This works for me. So we could pass "acpi=force" in EFI stub if it created the DT from scratch *and* ACPI tables are present (can it detect the latter? And maybe it could print something if none are available). If that works, the actual kernel can assume that ACPI needs to be explicitly enabled via acpi=force, irrespective of how much information it has in DT.

Ditto for me. I think this is a fine solution. And, yes, the stub can easily detect the presence of ACPI by looking in the UEFI config table.

I get the point behind doing this, but could we not have it pass in a different parameter than =force? Perhaps something new? I'd like to separate out the case that it was enabled automatically vs explicitly forced on by a user wanting to use ACPI on a system with both tables.

Ard had a point, so we should probably not pass acpi=force from EFI stub (especially since a user may explicitly pass acpi=off irrespective of DT presence). Some other property in the chosen node? It's not even an ABI since that's a contract between EFI stub and the rest of the kernel, so an in-kernel only interface.

On 2015年01月20日 02:01, Mark Rutland wrote:

On Mon, Jan 19, 2015 at 05:52:33PM +0000, Catalin Marinas wrote:

...

On Mon, Jan 19, 2015 at 04:59:47PM +0000, Jon Masters wrote:

...

On 01/19/2015 10:13 AM, Grant Likely wrote:

...

On Mon, 19 Jan 2015 13:51:45 +0000 , Catalin Marinas catalin.marinas@arm.com wrote:

...

On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote:

...

On 19 January 2015 at 11:42, Catalin Marinas catalin.marinas@arm.com wrote: > On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote: >> From: Al Stone al.stone@linaro.org >> >> Introduce one early parameters "off" and "force" for "acpi", acpi=off >> will be the default behavior for ARM64, so introduce acpi=force to >> enable ACPI on ARM64. >> >> Disable ACPI before early parameters parsed, and enable it to pass >> "acpi=force" if people want use ACPI on ARM64. This ensures DT be >> the prefer one if ACPI table and DT both are provided at this moment. > [...] >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -62,6 +62,7 @@ >> #include <asm/memblock.h> >> #include <asm/psci.h> >> #include <asm/efi.h> >> +#include <asm/acpi.h> >> >> unsigned int processor_id; >> EXPORT_SYMBOL(processor_id); >> @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) >> early_fixmap_init(); >> early_ioremap_init(); >> >> + disable_acpi(); >> + >> parse_early_param(); >> >> /* > > Did we get to any conclusion here? DT being the preferred one is fine > when both DT and ACPI are present but do we still want the kernel to > ignore ACPI altogether if DT is not present? It's a bit harder to detect > the presence of DT at this point since the EFI_STUB added one already. I > guess we could move the "acpi=force" argument passing to EFI_STUB if no > DT is present at boot.

Since the EFI stub populates the /chosen node in DT, I would prefer for it to add a property there to indicate whether it created the DT from scratch rather than adding ACPI specific stuff in there (even if it is just a string to concatenate)

This works for me. So we could pass "acpi=force" in EFI stub if it created the DT from scratch *and* ACPI tables are present (can it detect the latter? And maybe it could print something if none are available). If that works, the actual kernel can assume that ACPI needs to be explicitly enabled via acpi=force, irrespective of how much information it has in DT.

Ditto for me. I think this is a fine solution. And, yes, the stub can easily detect the presence of ACPI by looking in the UEFI config table.

I get the point behind doing this, but could we not have it pass in a different parameter than =force? Perhaps something new? I'd like to separate out the case that it was enabled automatically vs explicitly forced on by a user wanting to use ACPI on a system with both tables.

Ard had a point, so we should probably not pass acpi=force from EFI stub (especially since a user may explicitly pass acpi=off irrespective of DT presence). Some other property in the chosen node? It's not even an ABI since that's a contract between EFI stub and the rest of the kernel, so an in-kernel only interface.

Not strictly true once kexec is in place. Then it becomes a stub -> kernel -> kernel -> kernel -> ... interface, alnog with the rest of the properties the stub puts in the DTB.

Having something like /chosen/linux,uefi-stub-generated-dtb sounds sane regardless.

How about the patch (just RFC, maybe it is horrible :) ) below:

When system supporting both DT and ACPI but firmware providing no dtb, we can use this linux,uefi-stub-generated-dtb property to let kernel know that we can try ACPI configuration data.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org --- Documentation/devicetree/bindings/chosen.txt | 19 ++++++++++++++++ arch/arm64/kernel/setup.c | 34 +++++++++++++++++++++++++++- drivers/firmware/efi/libstub/fdt.c | 6 +++++ 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt index ed838f4..18776b9 100644 --- a/Documentation/devicetree/bindings/chosen.txt +++ b/Documentation/devicetree/bindings/chosen.txt @@ -44,3 +44,22 @@ Implementation note: Linux will look for the property "linux,stdout-path" or on PowerPC "stdout" if "stdout-path" is not found. However, the "linux,stdout-path" and "stdout" properties are deprecated. New platforms should only use the "stdout-path" property. + + +linux,uefi-stub-generated-dtb property +-------------------------------------- + +UEFI stub will generate this property in the chosen node to let linux kernel +know that there is no DTB provided by firmware. + +There is a use case for system supporting both DT and ACPI, when firmware +doesn't provide DT, we can try ACPI configration data to boot the system. + +Usage: + +linux,uefi-stub-generated-dtb = "true" means that it is true that the dtb +is generated by uefi stub + +or + +linux,uefi-stub-generated-dtb = "false" is the reverse. diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 54e39e3..8268c7b 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -371,6 +371,31 @@ static void __init request_standard_resources(void) } }

+int __init dt_scan_chosen(unsigned long node, const char *uname, + int depth, void *data) +{ + const char *p; + + if (depth != 1 || !data || + (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0)) + return 0; + + p = of_get_flat_dt_prop(node, "linux,uefi-stub-generated-dtb", NULL); + if (!p && !strcmp(p, "true")) + *data = true; + + return 1; +} + +static bool __init is_uefi_stub_generated_dtb(void) +{ + bool flag = false; + + of_scan_flat_dt(dt_scan_chosen, &flag); + + return flag; +} + u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };

void __init setup_arch(char **cmdline_p) @@ -389,7 +414,14 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init();

- disable_acpi(); + /* + * If no dtb provided by firmware, enable ACPI + * and try to boot with ACPI configuration data + */ + if (is_uefi_stub_generated_dtb()) + enable_acpi(); + else + disable_acpi();

parse_early_param();

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index c846a96..9e2084b 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -154,6 +154,12 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, if (status) goto fdt_set_fail;

+ /* Add a property to show the dtb is generated by uefi stub or not */ + status = fdt_setprop_string(fdt, node, "linux,uefi-stub-generated-dtb", + orig_fdt ? "false" : "true"); + if (status) + goto fdt_set_fail; + return EFI_SUCCESS;

fdt_set_fail:

On Tue, Jan 20, 2015 at 09:29:14AM +0000, Hanjun Guo wrote:

On 2015年01月20日 02:01, Mark Rutland wrote:

...

On Mon, Jan 19, 2015 at 05:52:33PM +0000, Catalin Marinas wrote:

...

On Mon, Jan 19, 2015 at 04:59:47PM +0000, Jon Masters wrote:

...

On 01/19/2015 10:13 AM, Grant Likely wrote:

...

On Mon, 19 Jan 2015 13:51:45 +0000 , Catalin Marinas catalin.marinas@arm.com wrote:

...

On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote: > On 19 January 2015 at 11:42, Catalin Marinas catalin.marinas@arm.com wrote: >> On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote: >>> From: Al Stone al.stone@linaro.org >>> >>> Introduce one early parameters "off" and "force" for "acpi", acpi=off >>> will be the default behavior for ARM64, so introduce acpi=force to >>> enable ACPI on ARM64. >>> >>> Disable ACPI before early parameters parsed, and enable it to pass >>> "acpi=force" if people want use ACPI on ARM64. This ensures DT be >>> the prefer one if ACPI table and DT both are provided at this moment. >> [...] >>> --- a/arch/arm64/kernel/setup.c >>> +++ b/arch/arm64/kernel/setup.c >>> @@ -62,6 +62,7 @@ >>> #include <asm/memblock.h> >>> #include <asm/psci.h> >>> #include <asm/efi.h> >>> +#include <asm/acpi.h> >>> >>> unsigned int processor_id; >>> EXPORT_SYMBOL(processor_id); >>> @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) >>> early_fixmap_init(); >>> early_ioremap_init(); >>> >>> + disable_acpi(); >>> + >>> parse_early_param(); >>> >>> /* >> >> Did we get to any conclusion here? DT being the preferred one is fine >> when both DT and ACPI are present but do we still want the kernel to >> ignore ACPI altogether if DT is not present? It's a bit harder to detect >> the presence of DT at this point since the EFI_STUB added one already. I >> guess we could move the "acpi=force" argument passing to EFI_STUB if no >> DT is present at boot. > > Since the EFI stub populates the /chosen node in DT, I would prefer > for it to add a property there to indicate whether it created the DT > from scratch rather than adding ACPI specific stuff in there (even if > it is just a string to concatenate)

This works for me. So we could pass "acpi=force" in EFI stub if it created the DT from scratch *and* ACPI tables are present (can it detect the latter? And maybe it could print something if none are available). If that works, the actual kernel can assume that ACPI needs to be explicitly enabled via acpi=force, irrespective of how much information it has in DT.

Ditto for me. I think this is a fine solution. And, yes, the stub can easily detect the presence of ACPI by looking in the UEFI config table.

I get the point behind doing this, but could we not have it pass in a different parameter than =force? Perhaps something new? I'd like to separate out the case that it was enabled automatically vs explicitly forced on by a user wanting to use ACPI on a system with both tables.

Ard had a point, so we should probably not pass acpi=force from EFI stub (especially since a user may explicitly pass acpi=off irrespective of DT presence). Some other property in the chosen node? It's not even an ABI since that's a contract between EFI stub and the rest of the kernel, so an in-kernel only interface.

Not strictly true once kexec is in place. Then it becomes a stub -> kernel -> kernel -> kernel -> ... interface, alnog with the rest of the properties the stub puts in the DTB.

Having something like /chosen/linux,uefi-stub-generated-dtb sounds sane regardless.

How about the patch (just RFC, maybe it is horrible :) ) below:

When system supporting both DT and ACPI but firmware providing no dtb, we can use this linux,uefi-stub-generated-dtb property to let kernel know that we can try ACPI configuration data.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

Documentation/devicetree/bindings/chosen.txt | 19 ++++++++++++++++ arch/arm64/kernel/setup.c | 34 +++++++++++++++++++++++++++- drivers/firmware/efi/libstub/fdt.c | 6 +++++ 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt index ed838f4..18776b9 100644 --- a/Documentation/devicetree/bindings/chosen.txt +++ b/Documentation/devicetree/bindings/chosen.txt @@ -44,3 +44,22 @@ Implementation note: Linux will look for the property "linux,stdout-path" or on PowerPC "stdout" if "stdout-path" is not found. However, the "linux,stdout-path" and "stdout" properties are deprecated. New platforms should only use the "stdout-path" property.

+linux,uefi-stub-generated-dtb property +--------------------------------------

+UEFI stub will generate this property in the chosen node to let linux kernel +know that there is no DTB provided by firmware.

+There is a use case for system supporting both DT and ACPI, when firmware +doesn't provide DT, we can try ACPI configration data to boot the system.

I don't think we need to list potential use cases here, this can be useful regardless of UEFI.

The other UEFI stub properties currently live under Documentation/arm/uefi.txt. This should live with them.

+Usage:

+linux,uefi-stub-generated-dtb = "true" means that it is true that the dtb +is generated by uefi stub

+or

+linux,uefi-stub-generated-dtb = "false" is the reverse.

I imagined this would be an empty property. It would only be present if the stub generated the DTB, and has no value:

/chosen { linux,uefi-stub-generated-dtb; };

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 54e39e3..8268c7b 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -371,6 +371,31 @@ static void __init request_standard_resources(void) } }

+int __init dt_scan_chosen(unsigned long node, const char *uname,

• 	int depth, void *data)
  

+{

• const char *p;
• if (depth != 1 || !data ||

•    (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
  

• return 0;
  

Do we ever generate chosen@0, and do we even accept that?

• p = of_get_flat_dt_prop(node, "linux,uefi-stub-generated-dtb", NULL);
• if (!p && !strcmp(p, "true"))

• *data = true;
  

• return 1;

+}

+static bool __init is_uefi_stub_generated_dtb(void) +{

• bool flag = false;
• of_scan_flat_dt(dt_scan_chosen, &flag);
• return flag;

+}

• u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID };

  void __init setup_arch(char **cmdline_p)

@@ -389,7 +414,14 @@ void __init setup_arch(char **cmdline_p) early_fixmap_init(); early_ioremap_init();

• disable_acpi();

• /*

• * If no dtb provided by firmware, enable ACPI
  

• * and try to boot with ACPI configuration data
  

• */
  

• if (is_uefi_stub_generated_dtb())

• enable_acpi();
  

• else

• disable_acpi();
  
  

  parse_early_param();

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index c846a96..9e2084b 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -154,6 +154,12 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt, if (status) goto fdt_set_fail;

• /* Add a property to show the dtb is generated by uefi stub or not */
• status = fdt_setprop_string(fdt, node, "linux,uefi-stub-generated-dtb",

• 		orig_fdt ? "false" : "true");
  

• if (status)

• goto fdt_set_fail;
  

This should create an empty property, and only when generated by the stub.

Thanks, Mark.

On Tue, Jan 20, 2015 at 11:10:32AM +0000, Mark Rutland wrote:

...

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 54e39e3..8268c7b 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -371,6 +371,31 @@ static void __init request_standard_resources(void) } }

+int __init dt_scan_chosen(unsigned long node, const char *uname,

• 	int depth, void *data)
  

+{

• const char *p;
• if (depth != 1 || !data ||

•    (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
  

• return 0;
  

Do we ever generate chosen@0, and do we even accept that?

This probably originates from some stupid cargo-culting on my side, based on some of the PPC-specific workarounds for old machines remaining in drivers/of/*. It should go.

/ Leif

On 21 January 2015 at 00:50, Stefano Stabellini stefano.stabellini@eu.citrix.com wrote:

On Tue, 20 Jan 2015, Hanjun Guo wrote:

...

On 2015年01月20日 02:01, Mark Rutland wrote:

...

On Mon, Jan 19, 2015 at 05:52:33PM +0000, Catalin Marinas wrote:

...

On Mon, Jan 19, 2015 at 04:59:47PM +0000, Jon Masters wrote:

...

On 01/19/2015 10:13 AM, Grant Likely wrote:

...

On Mon, 19 Jan 2015 13:51:45 +0000 , Catalin Marinas catalin.marinas@arm.com wrote: > On Mon, Jan 19, 2015 at 11:55:32AM +0000, Ard Biesheuvel wrote: > > On 19 January 2015 at 11:42, Catalin Marinas > > catalin.marinas@arm.com wrote: > > > On Wed, Jan 14, 2015 at 03:04:52PM +0000, Hanjun Guo wrote: > > > > From: Al Stone al.stone@linaro.org > > > > > > > > Introduce one early parameters "off" and "force" for "acpi", > > > > acpi=off > > > > will be the default behavior for ARM64, so introduce > > > > acpi=force to > > > > enable ACPI on ARM64. > > > > > > > > Disable ACPI before early parameters parsed, and enable it to > > > > pass > > > > "acpi=force" if people want use ACPI on ARM64. This ensures DT > > > > be > > > > the prefer one if ACPI table and DT both are provided at this > > > > moment. > > > [...] > > > > --- a/arch/arm64/kernel/setup.c > > > > +++ b/arch/arm64/kernel/setup.c > > > > @@ -62,6 +62,7 @@ > > > > #include <asm/memblock.h> > > > > #include <asm/psci.h> > > > > #include <asm/efi.h> > > > > +#include <asm/acpi.h> > > > > > > > > unsigned int processor_id; > > > > EXPORT_SYMBOL(processor_id); > > > > @@ -388,6 +389,8 @@ void __init setup_arch(char **cmdline_p) > > > > early_fixmap_init(); > > > > early_ioremap_init(); > > > > > > > > + disable_acpi(); > > > > + > > > > parse_early_param(); > > > > > > > > /* > > > > > > Did we get to any conclusion here? DT being the preferred one is > > > fine > > > when both DT and ACPI are present but do we still want the > > > kernel to > > > ignore ACPI altogether if DT is not present? It's a bit harder > > > to detect > > > the presence of DT at this point since the EFI_STUB added one > > > already. I > > > guess we could move the "acpi=force" argument passing to > > > EFI_STUB if no > > > DT is present at boot. > > > > Since the EFI stub populates the /chosen node in DT, I would > > prefer > > for it to add a property there to indicate whether it created the > > DT > > from scratch rather than adding ACPI specific stuff in there (even > > if > > it is just a string to concatenate) > > This works for me. So we could pass "acpi=force" in EFI stub if it > created the DT from scratch *and* ACPI tables are present (can it > detect > the latter? And maybe it could print something if none are > available). > If that works, the actual kernel can assume that ACPI needs to be > explicitly enabled via acpi=force, irrespective of how much > information > it has in DT.

Ditto for me. I think this is a fine solution. And, yes, the stub can easily detect the presence of ACPI by looking in the UEFI config table.

I get the point behind doing this, but could we not have it pass in a different parameter than =force? Perhaps something new? I'd like to separate out the case that it was enabled automatically vs explicitly forced on by a user wanting to use ACPI on a system with both tables.

Ard had a point, so we should probably not pass acpi=force from EFI stub (especially since a user may explicitly pass acpi=off irrespective of DT presence). Some other property in the chosen node? It's not even an ABI since that's a contract between EFI stub and the rest of the kernel, so an in-kernel only interface.

Not strictly true once kexec is in place. Then it becomes a stub -> kernel -> kernel -> kernel -> ... interface, alnog with the rest of the properties the stub puts in the DTB.

Having something like /chosen/linux,uefi-stub-generated-dtb sounds sane regardless.

How about the patch (just RFC, maybe it is horrible :) ) below:

When system supporting both DT and ACPI but firmware providing no dtb, we can use this linux,uefi-stub-generated-dtb property to let kernel know that we can try ACPI configuration data.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

Documentation/devicetree/bindings/chosen.txt | 19 ++++++++++++++++ arch/arm64/kernel/setup.c | 34 +++++++++++++++++++++++++++- drivers/firmware/efi/libstub/fdt.c | 6 +++++ 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt index ed838f4..18776b9 100644 --- a/Documentation/devicetree/bindings/chosen.txt +++ b/Documentation/devicetree/bindings/chosen.txt @@ -44,3 +44,22 @@ Implementation note: Linux will look for the property "linux,stdout-path" or on PowerPC "stdout" if "stdout-path" is not found. However, the "linux,stdout-path" and "stdout" properties are deprecated. New platforms should only use the "stdout-path" property.

+linux,uefi-stub-generated-dtb property +--------------------------------------

+UEFI stub will generate this property in the chosen node to let linux kernel +know that there is no DTB provided by firmware.

+There is a use case for system supporting both DT and ACPI, when firmware +doesn't provide DT, we can try ACPI configration data to boot the system.

+Usage:

+linux,uefi-stub-generated-dtb = "true" means that it is true that the dtb +is generated by uefi stub

+or

+linux,uefi-stub-generated-dtb = "false" is the reverse.

I am sorry to have to make the discussion even more complex than already is, however we have one more use case to consider: Linux booting on Xen as Dom0.

When booting as Dom0 on ACPI hardware, Linux doesn't have access to the UEFI firmware (no EFI stub). Xen passes a small device tree blob with a chosen node, memory information and a pointer to the ACPI tables. It looks similar to the DTB passed to Linux by the EFI stub but it is generated by Xen instead.

See also Christoffer's explanation:

http://marc.info/?l=linux-arm-kernel&m=142169977401658&w=2

The actual patches haven't been posted to the list yet, but Linaro seems to have an internal prototype working already. We really need to document this interface properly soon if we expect future Linux and Xen to adhere to it (hint hint).

In the Dom0 case Linux should parse the small device tree binary, then enable acpi.

Maybe we could generalize the concept of this uefi-stub-generated-dtb variable and call it instead "enable_acpi", optionally exporting the RDSP, like Parth's patches to Xen are doing (I haven't seen the code yet so I don't know the details).

Do you think that would work for you?

Here is the actual patch for booting linux on xen without uefi,

https://git.linaro.org/people/parth.dixit/xen-acpi-support/leg-kernel.git/co...

it is working and we are able to boot to linux from xen. We are doing this by passing RSDP pointer in skeleton device tree to linux and reading it as part of early boot initialization. As explained by stefano and christoffer we are not exposing UEFI so the EFI stub based approach will not work for us. Also we need to describe the memory in device tree as ACPI does not contain that information. Is it possible that we don't tie up EFI with ACPI ?

On Wed, 21 Jan 2015, Catalin Marinas wrote:

On Tue, Jan 20, 2015 at 07:20:06PM +0000, Stefano Stabellini wrote:

...

On Tue, 20 Jan 2015, Hanjun Guo wrote:

...

How about the patch (just RFC, maybe it is horrible :) ) below:

When system supporting both DT and ACPI but firmware providing no dtb, we can use this linux,uefi-stub-generated-dtb property to let kernel know that we can try ACPI configuration data.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

Documentation/devicetree/bindings/chosen.txt | 19 ++++++++++++++++ arch/arm64/kernel/setup.c | 34 +++++++++++++++++++++++++++- drivers/firmware/efi/libstub/fdt.c | 6 +++++ 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt index ed838f4..18776b9 100644 --- a/Documentation/devicetree/bindings/chosen.txt +++ b/Documentation/devicetree/bindings/chosen.txt @@ -44,3 +44,22 @@ Implementation note: Linux will look for the property "linux,stdout-path" or on PowerPC "stdout" if "stdout-path" is not found. However, the "linux,stdout-path" and "stdout" properties are deprecated. New platforms should only use the "stdout-path" property.

+linux,uefi-stub-generated-dtb property +--------------------------------------

+UEFI stub will generate this property in the chosen node to let linux kernel +know that there is no DTB provided by firmware.

+There is a use case for system supporting both DT and ACPI, when firmware +doesn't provide DT, we can try ACPI configration data to boot the system.

+Usage:

+linux,uefi-stub-generated-dtb = "true" means that it is true that the dtb +is generated by uefi stub

+or

+linux,uefi-stub-generated-dtb = "false" is the reverse.

I am sorry to have to make the discussion even more complex than already is, however we have one more use case to consider: Linux booting on Xen as Dom0.

When booting as Dom0 on ACPI hardware, Linux doesn't have access to the UEFI firmware (no EFI stub). Xen passes a small device tree blob with a chosen node, memory information and a pointer to the ACPI tables. It looks similar to the DTB passed to Linux by the EFI stub but it is generated by Xen instead.

We have two (or even three) different use cases here. One of them is a way to tell the kernel that there is no point in trying DT since it has been generated by the EFI stub and it doesn't have any SoC information. The kernel can bet on ACPI tables being present or just fail to boot. What I want to avoid is "acpi=force" parameter being part of the kernel/firmware ABI for using ACPI (whether DT is present or not).

We could call this something else ("linux,bare-dtb"?) if we want to use it in other situations but not "enable_acpi" as we don't want to move the ACPI enabling policy in the EFI stub or boot loader (or whatever generates the chosen node).

The Xen case is close to the kexec one. For the latter (I haven't looked at the current arm64 patches yet), I can see that you can pass an "acpi_rsdp" argument to the kernel with the ACPI RSDP address. I don't think that's ideal, we better have some defined DT bindings for such information in the absence of EFI. But it basically means that we'll allow ACPI on arm64 without EFI.

Good, I agree.

On Wed, 21 Jan 2015, Catalin Marinas wrote:

On Wed, Jan 21, 2015 at 03:29:52PM +0000, Jon Masters wrote:

...

On 01/21/2015 10:23 AM, Catalin Marinas wrote:

...

I have some questions for the ACPI and EFI folk:

1. When booting with ACPI, are the EFI run-time services required for anything? If yes, Xen may have a bigger problem

Yes. At least for some things. For example, installing an Operating System would require that you make runtime services calls to set the BootOrder/BootNext variables, and so on. Further, we use the GetTime service and EFI based reboot to avoid having special drivers. I had those added to SBBR as requirements for that reason.

So what would a kexec'ed kernel do here? Or we usually expect it to be short lived and doesn't need reboot, nor GetTime.

Xen is slightly more problematic but I wonder whether it could run a (paravirtualised) UEFI.

As a matter of fact that's how it is done on x86: Xen exports a paravirtualized UEFI run-time services interface (drivers/xen/efi.c). Daniel Kiper (CC'ed) wrote the code that went upstream last August. There is no reason why we could not do the same on ARM.

...

...

2. Could a boot loader (either kernel doing kexec or Xen) emulate the EFI system/config tables and still make them useful to the kernel but without EFI_BOOT or EFI_RUNTIME_SERVICES?

Yes. But again, without the other required pieces (including the services function pointers in the systab which are required) you'd crash soon after boot trying to make those calls.

My point was whether you can still pass information like RSDP address via EFI tables but explicitly disable runtime services so that the kernel won't try to make such calls (and crash).

I don't think that run-time services are going to be a problem for Xen, but I agree that it would be nice not to depend on them to have ACPI.

On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote:

Since PCI is not required in ACPI spec and ARM can run without it, introduce some stub functions to make PCI optional for ACPI, and make ACPI core run without CONFIG_PCI on ARM64.

When PCI is enabled on ARM64, ACPI core will need some PCI functions to make it functional, so introduce some empty functions here and implement it later.

Since ACPI on X86 and IA64 depends on PCI and this patch only makes PCI optional for ARM64, it will not break anything on X86 and IA64.

Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

Is this patch still required, now that we have PCI for arm64? I know the ACPI spec doesn't require PCI but do we expect any arm64 servers aimed at ACPI without PCIe?

Anyway, that's not the main point, see more below.

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h index 872ba93..fded096 100644 --- a/arch/arm64/include/asm/pci.h +++ b/arch/arm64/include/asm/pci.h @@ -24,6 +24,12 @@ */ #define PCI_DMA_BUS_IS_PHYS (0) +static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel) +{

• /* no legacy IRQ on arm64 */
• return -ENODEV;

+}

extern int isa_dma_bridge_buggy; #ifdef CONFIG_PCI diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index ce5836c..42fb195 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -10,6 +10,7 @@

*/ +#include <linux/acpi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/kernel.h> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) bus->domain_nr = domain; } #endif

+/*

• • raw_pci_read/write - Platform-specific PCI config space access.
• • Default empty implementation. Replace with an architecture-specific setup
• • routine, if necessary.
• */

+int raw_pci_read(unsigned int domain, unsigned int bus,

•   unsigned int devfn, int reg, int len, u32 *val)
  

+{

• return -EINVAL;

+}

+int raw_pci_write(unsigned int domain, unsigned int bus,

• unsigned int devfn, int reg, int len, u32 val)
  

+{

• return -EINVAL;

+}

+#ifdef CONFIG_ACPI +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{

• /* TODO: Should be revisited when implementing PCI on ACPI */
• return NULL;

+} +#endif

Do these functions have anything to do with the subject? You add them in arch/arm64/kernel/pci.c which is compiled only when CONFIG_PCI while the commit log implies that you add them to allow CONFIG_PCI to be off.

When PCI is enabled and the above functions are compiled in, do they need to return any useful data or just -EINVAL. Are they ever called?

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 39f3ec1..c346011 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -43,7 +43,7 @@ acpi-y += processor_core.o acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o acpi-y += ec.o acpi-$(CONFIG_ACPI_DOCK) += dock.o -acpi-y += pci_root.o pci_link.o pci_irq.o +acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o acpi-y += acpi_lpss.o acpi-y += acpi_platform.o acpi-y += acpi_pnp.o diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 163e82f..c5ff8ba 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -26,8 +26,13 @@ acpi_status acpi_os_initialize1(void); int init_acpi_device_notify(void); int acpi_scan_init(void); +#ifdef CONFIG_PCI void acpi_pci_root_init(void); void acpi_pci_link_init(void); +#else +static inline void acpi_pci_root_init(void) {} +static inline void acpi_pci_link_init(void) {} +#endif void acpi_processor_init(void); void acpi_platform_init(void); void acpi_pnp_init(void);

That's a good clean-up.

diff --git a/include/linux/pci.h b/include/linux/pci.h index 360a966..1476a66 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -564,15 +564,6 @@ struct pci_ops { int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val); }; -/*

• • ACPI needs to be able to access PCI config space before we've done a
• • PCI bus scan and created pci_bus structures.
• */

-int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,

•  int reg, int len, u32 *val);
  

-int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,

•   int reg, int len, u32 val);
  

struct pci_bus_region { dma_addr_t start; dma_addr_t end; @@ -1329,6 +1320,16 @@ typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode, unsigned int command_bits, u32 flags); void pci_register_set_vga_state(arch_set_vga_state_t func); +/*

• • ACPI needs to be able to access PCI config space before we've done a
• • PCI bus scan and created pci_bus structures.
• */

+int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,

•  int reg, int len, u32 *val);
  

+int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,

•   int reg, int len, u32 val);
  

+void pcibios_penalize_isa_irq(int irq, int active);

#else /* CONFIG_PCI is not enabled */ /* @@ -1430,6 +1431,23 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, unsigned int devfn) { return NULL; } +static inline struct pci_bus *pci_find_bus(int domain, int busnr) +{ return NULL; }

+static inline int pci_bus_write_config_byte(struct pci_bus *bus,

• 		unsigned int devfn, int where, u8 val)
  

+{ return -ENOSYS; }

+static inline int raw_pci_read(unsigned int domain, unsigned int bus,

• 	unsigned int devfn, int reg, int len, u32 *val)
  

+{ return -ENOSYS; }

+static inline int raw_pci_write(unsigned int domain, unsigned int bus,

• 	unsigned int devfn, int reg, int len, u32 val)
  

+{ return -ENOSYS; }

So you implement the !CONFIG_PCI functions here to return -ENOSYS while the arm64 CONFIG_PCI ones would return -EINVAL. I'm confused.

Hi Folks,

Sorry for top posting from bed. The mainstream servers will all likely do PCIe but there are several that may not. They should not be excluded. That said, if we booted a previously built kernel on a system without an MCFG and got no ECAM/root then things would probably still work.

I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec.

Jon.

On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote:

On 2015年01月16日 17:49, Catalin Marinas wrote:

...

On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote:

...

--- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -10,6 +10,7 @@

*/

+#include <linux/acpi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/kernel.h> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) bus->domain_nr = domain; } #endif

+/*

• • raw_pci_read/write - Platform-specific PCI config space access.
• • Default empty implementation. Replace with an architecture-specific setup
• • routine, if necessary.
• */

+int raw_pci_read(unsigned int domain, unsigned int bus,

•   unsigned int devfn, int reg, int len, u32 *val)
  

+{

• return -EINVAL;

+}

+int raw_pci_write(unsigned int domain, unsigned int bus,

• unsigned int devfn, int reg, int len, u32 val)
  

+{

• return -EINVAL;

+}

+#ifdef CONFIG_ACPI +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{

• /* TODO: Should be revisited when implementing PCI on ACPI */
• return NULL;

+} +#endif

[...]

...

When PCI is enabled and the above functions are compiled in, do they need to return any useful data or just -EINVAL. Are they ever called?

They will be called if PCI root bridge is defined in DSDT, should I print some warning message before it is implemented?

My point: do they need to return real data when a PCI root bridge is defined in DSDT or you always expect them to always return some -E*? Can you explain why?

On Tue, Jan 20, 2015 at 02:39:16AM +0000, Hanjun Guo wrote:

On 2015年01月19日 18:42, Catalin Marinas wrote:

...

On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote:

...

On 2015年01月16日 17:49, Catalin Marinas wrote:

...

On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote:

...

--- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -10,6 +10,7 @@ * */

+#include <linux/acpi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/kernel.h> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) bus->domain_nr = domain; } #endif

+/*

• • raw_pci_read/write - Platform-specific PCI config space access.
• • Default empty implementation. Replace with an architecture-specific setup
• • routine, if necessary.
• */

+int raw_pci_read(unsigned int domain, unsigned int bus,

•   unsigned int devfn, int reg, int len, u32 *val)
  

+{

• return -EINVAL;

+}

+int raw_pci_write(unsigned int domain, unsigned int bus,

• unsigned int devfn, int reg, int len, u32 val)
  

+{

• return -EINVAL;

+}

+#ifdef CONFIG_ACPI +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{

• /* TODO: Should be revisited when implementing PCI on ACPI */
• return NULL;

+} +#endif

[...]

...

...

When PCI is enabled and the above functions are compiled in, do they need to return any useful data or just -EINVAL. Are they ever called?

They will be called if PCI root bridge is defined in DSDT, should I print some warning message before it is implemented?

My point: do they need to return real data when a PCI root bridge is defined in DSDT or you always expect them to always return some -E*? Can you explain why?

Not always return -E* or NULL;

For raw_pci_read/write(), they are needed to access the PCI config space before the PCI root bus is created. so they will return 0 if access to PCI config space is ok; pci_acpi_scan_root() will return root bus pointer if it is successfully created.

OK. So what's the plan for implementing these functions properly. For the raw_pci_read/write, the comment states "replace with an architecture-specific setup routine". What does this mean?

For pci_acpi_scan_root(), at least the comment states a "TODO". Is there anyone working on this or we don't expect servers with PCIe soon?

On 20.01.2015 12:00, Catalin Marinas wrote:

On Tue, Jan 20, 2015 at 02:39:16AM +0000, Hanjun Guo wrote:

...

On 2015年01月19日 18:42, Catalin Marinas wrote:

...

On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote:

...

On 2015年01月16日 17:49, Catalin Marinas wrote:

...

On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote:

...

--- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -10,6 +10,7 @@ * */

+#include <linux/acpi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/kernel.h> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) bus->domain_nr = domain; } #endif

+/*

• • raw_pci_read/write - Platform-specific PCI config space access.
• • Default empty implementation. Replace with an architecture-specific setup
• • routine, if necessary.
• */

+int raw_pci_read(unsigned int domain, unsigned int bus,

•   unsigned int devfn, int reg, int len, u32 *val)
  

+{

• return -EINVAL;

+}

+int raw_pci_write(unsigned int domain, unsigned int bus,

• unsigned int devfn, int reg, int len, u32 val)
  

+{

• return -EINVAL;

+}

+#ifdef CONFIG_ACPI +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{

• /* TODO: Should be revisited when implementing PCI on ACPI */
• return NULL;

+} +#endif

[...]

...

...

When PCI is enabled and the above functions are compiled in, do they need to return any useful data or just -EINVAL. Are they ever called?

They will be called if PCI root bridge is defined in DSDT, should I print some warning message before it is implemented?

My point: do they need to return real data when a PCI root bridge is defined in DSDT or you always expect them to always return some -E*? Can you explain why?

Not always return -E* or NULL;

For raw_pci_read/write(), they are needed to access the PCI config space before the PCI root bus is created. so they will return 0 if access to PCI config space is ok; pci_acpi_scan_root() will return root bus pointer if it is successfully created.

OK. So what's the plan for implementing these functions properly. For the raw_pci_read/write, the comment states "replace with an architecture-specific setup routine". What does this mean?

raw_pci_read/write will use MMCONFIG code to access PCI config space. Please see my patch set: http://lkml.iu.edu/hypermail/linux/kernel/1411.2/02753.html which is going to refactor the x86 specific code so it would be usable for ARM64 too.

For pci_acpi_scan_root(), at least the comment states a "TODO". Is there anyone working on this or we don't expect servers with PCIe soon?

We do, Cavium, AMD and APM boards have PCIe.

Me and Mark Salter have posted initial support for ACPI PCI probe: http://lkml.iu.edu/hypermail/linux/kernel/1411.0/05026.html http://lists.linaro.org/pipermail/linaro-acpi/2014-November/002970.html

Regards, Tomasz

On Wed, Jan 14, 2015 at 03:04:55PM +0000, Hanjun Guo wrote:

FADT Major.Minor version was introduced in ACPI 5.1, it is the same as ACPI version.

In ACPI 5.1, some major gaps are fixed for ARM, such as updates in MADT table for GIC and SMP init, without those updates, we can not get the MPIDR for SMP init, and GICv2/3 related init information, so we can't boot arm64 ACPI properly with table versions predating 5.1.

If firmware provides ACPI tables with ACPI version less than 5.1, OS will be messed up with those information and have no way to init

Nit: "that information"

or

"...OS has no way to retrieve the configuration data that is necessary to init SMP boot protocol and the GIC properly, so.."

smp and GIC, so disable ACPI if we get an FADT table with version less that 5.1.

Patch should be reordered in the series and must be sequenced before patch 5 for bisectability (that patch implements DT unflattening if ACPI is disabled), or squashed with previous patches.

Lorenzo

Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/kernel/acpi.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 39a1655..4177758 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -13,6 +13,8 @@

• published by the Free Software Foundation.

*/ +#define pr_fmt(fmt) "ACPI: " fmt

#include <linux/init.h> #include <linux/acpi.h> #include <linux/cpumask.h> @@ -49,10 +51,32 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); } +static int __init acpi_parse_fadt(struct acpi_table_header *table) +{

• struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
• /*

• * Revision in table header is the FADT Major revision,
  

• * and there is a minor revision of FADT which was introduced
  

• * by ACPI 5.1, we only deal with ACPI 5.1 or newer revision
  

• * to get arm boot flags, or we will disable ACPI.
  

• */
  

• if (table->revision > 5 ||

•    (table->revision == 5 && fadt->minor_revision >= 1))
  

• return 0;
  

• pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",

• table->revision, fadt->minor_revision);
  

• disable_acpi();
• return -EINVAL;

+}

/*

• acpi_boot_table_init() called from setup_arch(), always.
• 1. find RSDP and get its address, and then find XSDT
• 2. extract all tables and checksums them all

• • 3. check ACPI FADT revision
  • We can parse ACPI boot-time tables such as MADT after
  • this function is called.

@@ -64,8 +88,13 @@ void __init acpi_boot_table_init(void) return; /* Initialize the ACPI boot-time table parser. */

• if (acpi_table_init())

• if (acpi_table_init()) { disable_acpi();

• return;
  

• }
• if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt))

• pr_err("Can't find FADT or error happened during parsing FADT\n");
  

} static int __init parse_acpi(char *arg) -- 1.9.1

On Wed, Jan 14, 2015 at 03:04:55PM +0000, Hanjun Guo wrote:

--- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c

[...]

@@ -64,8 +88,13 @@ void __init acpi_boot_table_init(void) return; /* Initialize the ACPI boot-time table parser. */

• if (acpi_table_init())

• if (acpi_table_init()) { disable_acpi();

• return;
  

• }
• if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt))

• pr_err("Can't find FADT or error happened during parsing FADT\n");
  

}

Do you need a disable_acpi() call here as well?

Hi Hanjun,

On Wed, Jan 14, 2015 at 03:04:58PM +0000, Hanjun Guo wrote:

[...]

diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index 780f82c..bf22650 100644 --- a/arch/arm64/include/asm/smp.h +++ b/arch/arm64/include/asm/smp.h @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec); extern void handle_IPI(int ipinr, struct pt_regs *regs);

/*

• • Setup the set of possible CPUs (via set_cpu_possible)

• • Discover the set of possible CPUs and determine their
• • SMP operations.
  */

-extern void smp_init_cpus(void); +extern void of_smp_init_cpus(void);

/*

• Provide a function to raise an IPI cross call on CPUs in callmap.

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 4177758..55d99d9 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -24,6 +24,10 @@ #include <linux/bootmem.h> #include <linux/smp.h>

+#include <asm/smp_plat.h> +#include <asm/cputype.h> +#include <asm/cpu_ops.h>

Alphabetical order please.

int acpi_noirq; /* skip ACPI IRQ initialization */ int acpi_disabled; EXPORT_SYMBOL(acpi_disabled); @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled); int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled);

+static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */

Needed for hotplug ? Remember that each patch must be self-contained.

This patch is clearly not, if I did not know you need to carry this stuff over after boot for physical cpu hotplugging it would be hard to understand most of this code.

Think about that for anyone looking at your commit-log as a stand alone entity in the kernel history.

/*

• __acpi_map_table() will be called before page_init(), so early_ioremap()
• or early_memremap() should be called here to for ACPI table mapping.

@@ -51,6 +57,134 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); }

+/**

• • acpi_map_gic_cpu_interface - generates a logical cpu number
• • and map to MPIDR represented by GICC structure
• • @mpidr: CPU's hardware id to register, MPIDR represented in MADT
• • @enabled: this cpu is enabled or not
• • Returns the logical cpu number which maps to MPIDR
• */

+static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled)

It would be __init, except that you might need it after boot for hotplug, correct ?

+{

•   int cpu;
  

•   if (mpidr == INVALID_HWID) {
  

•           pr_info("Skip MADT cpu entry with invalid MPIDR\n");
  

•           return -EINVAL;
  

•   }
  

•   total_cpus++;
  

•   if (!enabled)
  

•           return -EINVAL;
  

•   if (enabled_cpus >=  NR_CPUS) {
  

•           pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
  

•                   NR_CPUS, total_cpus, mpidr);
  

•           return -EINVAL;
  

•   }
  

•   /* No need to check duplicate MPIDRs for the first CPU */
  

•   if (enabled_cpus) {
  

•           /*
  

•            * Duplicate MPIDRs are a recipe for disaster. Scan
  

•            * all initialized entries and check for
  

•            * duplicates. If any is found just ignore the CPU.
  

•            */
  

•           for_each_possible_cpu(cpu) {
  

•                   if (cpu_logical_map(cpu) == mpidr) {
  

•                           pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
  

•                                  mpidr);
  

•                           return -EINVAL;
  

•                   }
  

•           }
  

•           /* allocate a logical cpu id for the new comer */
  

•           cpu = cpumask_next_zero(-1, cpu_possible_mask);
  

•   } else {
  

•           /*
  

•            * First GICC entry must be BSP as ACPI spec said
  

•            * in section 5.2.12.15
  

•            */
  

•           if  (cpu_logical_map(0) != mpidr) {
  

•                   pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
  

•                          mpidr);
  

•                   return -EINVAL;
  

•           }
  

•           /*
  

•            * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

You mean cpu_possible_mask ? That's what you allocate from above.

•            * for BSP, no need to allocate again.
  

•            */
  

•           cpu = 0;
  

•   }
  

•   /* CPU 0 was already initialized */
  

•   if (cpu) {
  

•           cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
  

•           if (!cpu_ops[cpu])
  

•                   return -EINVAL;
  

•           if (cpu_ops[cpu]->cpu_init(NULL, cpu))
  

•                   return -EOPNOTSUPP;
  

•           /* map the logical cpu id to cpu MPIDR */
  

•           cpu_logical_map(cpu) = mpidr;
  

•           set_cpu_possible(cpu, true);
  

•   } else {
  

•           /* get cpu0's ops, no need to return if ops is null */
  

•           cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
  

•   }
  

I do not see much point in calling cpu_get_ops with NULL, and adding the check in it to return NULL when the parameter is NULL.

What would you expect from cpu_get_ops when called with NULL other than a NULL pointer ?

You could move:

cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);

out of the if and remove the else, do not know if it makes code clearer, shorter for certain.

•   enabled_cpus++;
  

•   return cpu;
  

+}

+static int __init +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,

•                           const unsigned long end)
  

+{

•   struct acpi_madt_generic_interrupt *processor;
  

•   processor = (struct acpi_madt_generic_interrupt *)header;
  

•   if (BAD_MADT_ENTRY(processor, end))
  

•           return -EINVAL;
  

•   acpi_table_print_madt_entry(header);
  

•   acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK,
  

•           processor->flags & ACPI_MADT_ENABLED);
  

•   return 0;
  

+}

+/* Parse GIC cpu interface entries in MADT for SMP init */ +void __init acpi_smp_init_cpus(void) +{

•   int count;
  

•   /*
  

•    * do a partial walk of MADT to determine how many CPUs
  

•    * we have including disabled CPUs, and get information
  

•    * we need for SMP init
  

•    */
  

•   count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
  

•                   acpi_parse_gic_cpu_interface, 0);
  

•   if (!count) {
  

•           pr_err("No GIC CPU interface entries present\n");
  

•           return;
  

•   } else if (count < 0) {
  

•           pr_err("Error parsing GIC CPU interface entry\n");
  

•           return;
  

•   }
  

•   /* Make boot-up look pretty */
  

•   pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
  

+}

static int __init acpi_parse_fadt(struct acpi_table_header *table) { struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; @@ -62,8 +196,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) * to get arm boot flags, or we will disable ACPI. */ if (table->revision > 5 ||

•       (table->revision == 5 && fadt->minor_revision >= 1))
  

•           return 0;
  

•       (table->revision == 5 && fadt->minor_revision >= 1)) {
  

•           /*
  

•            * ACPI 5.1 only has two explicit methods to boot up SMP,
  

•            * PSCI and Parking protocol, but the Parking protocol is
  

•            * only specified for ARMv7 now, so make PSCI as the only
  

•            * way for the SMP boot protocol before some updates for
  

•            * the ACPI spec or the Parking protocol spec.
  

•            */
  

•           if (acpi_psci_present())
  

•                   return 0;
  

•           pr_warn("No PSCI support, will not bring up secondary CPUs\n");
  

•           return -EOPNOTSUPP;
  

•   }
  
    pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
            table->revision, fadt->minor_revision);
  

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index cce9524..1ea7b9f 100644 --- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops;

const struct cpu_operations *cpu_ops[NR_CPUS];

-static const struct cpu_operations *supported_cpu_ops[] __initconst = { +static const struct cpu_operations *supported_cpu_ops[] = {

This __initconst removal should be explained either with code needing it or through a comment. You can't make changes with future patches in mind, since they may never get merged and you leave code in this patch incomplete.

As far as I know if physical CPU hotplug can't/won't be done on ARM64 your patch would make changes that are not needed, and miss some changes that are (eg removing enabled_cpus or make it __initdata).

You can't write a patch with assumptions on subsequent patches.

#ifdef CONFIG_SMP &smp_spin_table_ops, #endif @@ -35,10 +35,13 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = { NULL, };

-static const struct cpu_operations * __init cpu_get_ops(const char *name) +const struct cpu_operations *cpu_get_ops(const char *name)

Ditto.

{ const struct cpu_operations **ops = supported_cpu_ops;

•   if (!name)
  

•           return NULL;
  

See above.

    while (*ops) {
            if (!strcmp(name, (*ops)->name))
                    return *ops;

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index ef5b1e1..54e39e3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -414,13 +414,16 @@ void __init setup_arch(char **cmdline_p) if (acpi_disabled) { unflatten_device_tree(); psci_dt_init();

•           cpu_read_bootcpu_ops();
  

+#ifdef CONFIG_SMP

•           of_smp_init_cpus();
  

+#endif } else { psci_acpi_init();

•           acpi_smp_init_cpus();
  

With DT you call cpu_read_bootcpu_ops() and then of_smp_init_cpus() with acpi you have one function that does both, it is not really neat.

Thanks, Lorenzo

    }

•   cpu_read_bootcpu_ops();
  

#ifdef CONFIG_SMP

•   smp_init_cpus();
    smp_build_mpidr_hash();
  

#endif

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 7ae6ee0..5aaf5a4 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -323,7 +323,7 @@ void __init smp_prepare_boot_cpu(void)

• cpu logical map array containing MPIDR values related to logical
• cpus. Assumes that cpu_logical_map(0) has already been initialized.

*/ -void __init smp_init_cpus(void) +void __init of_smp_init_cpus(void) { struct device_node *dn = NULL; unsigned int i, cpu = 1; -- 1.9.1

On Tue, Jan 20, 2015 at 01:09:55PM +0000, Hanjun Guo wrote:

[...]

...

...

+{

•   int cpu;
  

•   if (mpidr == INVALID_HWID) {
  

•           pr_info("Skip MADT cpu entry with invalid MPIDR\n");
  

•           return -EINVAL;
  

•   }
  

•   total_cpus++;
  

•   if (!enabled)
  

•           return -EINVAL;
  

•   if (enabled_cpus >=  NR_CPUS) {
  

•           pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
  

•                   NR_CPUS, total_cpus, mpidr);
  

•           return -EINVAL;
  

•   }
  

•   /* No need to check duplicate MPIDRs for the first CPU */
  

•   if (enabled_cpus) {
  

•           /*
  

•            * Duplicate MPIDRs are a recipe for disaster. Scan
  

•            * all initialized entries and check for
  

•            * duplicates. If any is found just ignore the CPU.
  

•            */
  

•           for_each_possible_cpu(cpu) {
  

•                   if (cpu_logical_map(cpu) == mpidr) {
  

•                           pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
  

•                                  mpidr);
  

•                           return -EINVAL;
  

•                   }
  

•           }
  

•           /* allocate a logical cpu id for the new comer */
  

•           cpu = cpumask_next_zero(-1, cpu_possible_mask);
  

•   } else {
  

•           /*
  

•            * First GICC entry must be BSP as ACPI spec said
  

•            * in section 5.2.12.15
  

•            */
  

•           if  (cpu_logical_map(0) != mpidr) {
  

•                   pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
  

•                          mpidr);
  

•                   return -EINVAL;
  

•           }
  

•           /*
  

•            * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

You mean cpu_possible_mask ? That's what you allocate from above.

Another hot-plug piece leaved, will update it.

...

...

•            * for BSP, no need to allocate again.
  

•            */
  

•           cpu = 0;
  

•   }
  

•   /* CPU 0 was already initialized */
  

•   if (cpu) {
  

•           cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
  

•           if (!cpu_ops[cpu])
  

•                   return -EINVAL;
  

•           if (cpu_ops[cpu]->cpu_init(NULL, cpu))
  

•                   return -EOPNOTSUPP;
  

•           /* map the logical cpu id to cpu MPIDR */
  

•           cpu_logical_map(cpu) = mpidr;
  

•           set_cpu_possible(cpu, true);
  

•   } else {
  

•           /* get cpu0's ops, no need to return if ops is null */
  

•           cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
  

•   }
  

I do not see much point in calling cpu_get_ops with NULL, and adding the check in it to return NULL when the parameter is NULL.

What would you expect from cpu_get_ops when called with NULL other than a NULL pointer ?

I'm lost here since it is best way for the implementation I think, any suggestions?

My suggestion is: no PSCI, no secondaries booting, that's what your code wants to achieve, right ? What's the point in calling cpu_get_ops() when PSCI is not present then ? What do you expect from calling cpu_get_ops(NULL) other than a NULL pointer in return ?

Put it differently, if !acpi_psci_present() parsing code should bail out, there are no CPU ops to initialize for secondaries, that's what I think your aim is, correct ?

On a side note, if ACPI PSCI is not present you still keep booting on the boot processor.

What piece of code initialize cpu_ops[0] in that case ? I could not find any, basically you would run the kernel with cpu_ops[0] == NULL.

...

You could move:

cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);

out of the if and remove the else, do not know if it makes code clearer, shorter for certain.

...

•   enabled_cpus++;
  

•   return cpu;
  

+}

+static int __init +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,

•                           const unsigned long end)
  

+{

•   struct acpi_madt_generic_interrupt *processor;
  

•   processor = (struct acpi_madt_generic_interrupt *)header;
  

•   if (BAD_MADT_ENTRY(processor, end))
  

•           return -EINVAL;
  

•   acpi_table_print_madt_entry(header);
  

•   acpi_map_gic_cpu_interface(processor->arm_mpidr & MPIDR_HWID_BITMASK,
  

•           processor->flags & ACPI_MADT_ENABLED);
  

•   return 0;
  

+}

+/* Parse GIC cpu interface entries in MADT for SMP init */ +void __init acpi_smp_init_cpus(void) +{

•   int count;
  

•   /*
  

•    * do a partial walk of MADT to determine how many CPUs
  

•    * we have including disabled CPUs, and get information
  

•    * we need for SMP init
  

•    */
  

•   count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
  

•                   acpi_parse_gic_cpu_interface, 0);
  

•   if (!count) {
  

•           pr_err("No GIC CPU interface entries present\n");
  

•           return;
  

•   } else if (count < 0) {
  

•           pr_err("Error parsing GIC CPU interface entry\n");
  

•           return;
  

•   }
  

•   /* Make boot-up look pretty */
  

•   pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
  

+}

• static int __init acpi_parse_fadt(struct acpi_table_header *table) { struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;

@@ -62,8 +196,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) * to get arm boot flags, or we will disable ACPI. */ if (table->revision > 5 ||

•       (table->revision == 5 && fadt->minor_revision >= 1))
  

•           return 0;
  

•       (table->revision == 5 && fadt->minor_revision >= 1)) {
  

•           /*
  

•            * ACPI 5.1 only has two explicit methods to boot up SMP,
  

•            * PSCI and Parking protocol, but the Parking protocol is
  

•            * only specified for ARMv7 now, so make PSCI as the only
  

•            * way for the SMP boot protocol before some updates for
  

•            * the ACPI spec or the Parking protocol spec.
  

•            */
  

•           if (acpi_psci_present())
  

•                   return 0;
  

•           pr_warn("No PSCI support, will not bring up secondary CPUs\n");
  

•           return -EOPNOTSUPP;
  

•   }
  
     pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
             table->revision, fadt->minor_revision);
  

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index cce9524..1ea7b9f 100644 --- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops;

const struct cpu_operations *cpu_ops[NR_CPUS];

-static const struct cpu_operations *supported_cpu_ops[] __initconst = { +static const struct cpu_operations *supported_cpu_ops[] = {

This __initconst removal should be explained either with code needing it or through a comment. You can't make changes with future patches in mind, since they may never get merged and you leave code in this patch incomplete.

As far as I know if physical CPU hotplug can't/won't be done on ARM64 your patch would make changes that are not needed, and miss some changes that are (eg removing enabled_cpus or make it __initdata).

I agree with you :)

...

You can't write a patch with assumptions on subsequent patches.

...

#ifdef CONFIG_SMP &smp_spin_table_ops, #endif @@ -35,10 +35,13 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = { NULL, };

-static const struct cpu_operations * __init cpu_get_ops(const char *name) +const struct cpu_operations *cpu_get_ops(const char *name)

Ditto.

...

{ const struct cpu_operations **ops = supported_cpu_ops;

•   if (!name)
  

•           return NULL;
  

See above.

...

     while (*ops) {
             if (!strcmp(name, (*ops)->name))
                     return *ops;

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index ef5b1e1..54e39e3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -414,13 +414,16 @@ void __init setup_arch(char **cmdline_p) if (acpi_disabled) { unflatten_device_tree(); psci_dt_init();

•           cpu_read_bootcpu_ops();
  

+#ifdef CONFIG_SMP

•           of_smp_init_cpus();
  

+#endif } else { psci_acpi_init();

•           acpi_smp_init_cpus();
  

With DT you call cpu_read_bootcpu_ops() and then of_smp_init_cpus() with acpi you have one function that does both, it is not really neat.

The mechanism for ACPI table entry scanning is that for every matched structure (such as GICC) found, the parse function will be called, so if we separate them it will duplicate the scanning of ACPI tables.

Call it acpi_init_cpus() then.

Lorenzo

On 2015年01月20日 19:17, Catalin Marinas wrote:

On Wed, Jan 14, 2015 at 03:04:59PM +0000, Hanjun Guo wrote:

...

--- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -64,6 +64,38 @@ static int map_lsapic_id(struct acpi_subtable_header *entry, return 0; }

+/*

• • On ARM platform, MPIDR value is the hardware ID as apic ID
• • on Intel platforms
• */

+static int map_gicc_mpidr(struct acpi_subtable_header *entry,

• int device_declaration, u32 acpi_id, int *mpidr)
  

+{

• struct acpi_madt_generic_interrupt *gicc =

•    container_of(entry, struct acpi_madt_generic_interrupt, header);
  

• if (!(gicc->flags & ACPI_MADT_ENABLED))

• return -ENODEV;
  

• /* In the GIC interrupt model, logical processors are

• * required to have a Processor Device object in the DSDT,
  

• * so we should check device_declaration here
  

• */
  

• if (device_declaration && (gicc->uid == acpi_id)) {

• /*
  

•  * Only bits [0:7] Aff0, bits [8:15] Aff1, bits [16:23] Aff2
  

•  * and bits [32:39] Aff3 are meaningful, so pack the Affx
  

•  * fields into a single 32 bit identifier to accommodate the
  

•  * acpi processor drivers.
  

•  */
  

• *mpidr = ((gicc->arm_mpidr & 0xff00000000) >> 8)
  

• 	 | gicc->arm_mpidr;
  

Nitpick: I would clarify the comment here since "meaningful" doesn't necessarily mean that the other bits are 0. And if they are not, the above masking is not enough. Looking at the ACPI 5.1 spec, bits other than Aff[0-3] must be zero.

Same for the pack_mpidr() comment.

OK, how about:

/* * bits other than [0:7] Aff0, [8:15] Aff1, [16:23] Aff2 and * [32:39] Aff3 must be 0 which is defined in ACPI 5.1, so pack * the Affx fields into a single 32 bit identifier to accommodate * the acpi processor drivers. */

Thanks Hanjun

On 14/01/15 15:05, Hanjun Guo wrote:

Introduce ACPI_IRQ_MODEL_GIC which is needed for ARM64 as GIC is used, and then register device's gsi with the core IRQ subsystem.

acpi_register_gsi() is similar to DT based irq_of_parse_and_map(), since gsi is unique in the system, so use hwirq number directly for the mapping.

Originally-by: Amit Daniel Kachhap amit.daniel@samsung.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/kernel/acpi.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/acpi/bus.c | 3 ++ include/linux/acpi.h | 1 + 3 files changed, 77 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 0b7c3a6..c3e24c4 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -37,6 +37,12 @@ EXPORT_SYMBOL(acpi_pci_disabled); static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */ /*

• • Since we're on ARM, the default interrupt routing model
• • clearly has to be GIC.
• */

+enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;

+/*

• __acpi_map_table() will be called before page_init(), so early_ioremap()
• or early_memremap() should be called here to for ACPI table mapping.

*/ @@ -184,6 +190,73 @@ void __init acpi_smp_init_cpus(void) pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); } +int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = irq_find_mapping(NULL, gsi);

Is the ACPI implementation really expecting to only deal with a single domain for interrupts?

This is likely to lead to a number of issues as we're now implementing stacked domains (GICv2m, GICv3 ITS are already using that model, and "wire-to MSI translators" will certainly do the same).

This means that, despite having a single GIC described in ACPI, you may end-up with multiple domains. I expect you'll be safe with MSI (assuming you never describe them in ACPI), but any form of wire-to-MSI translation won't work at all.

Thanks,

M.