2018-03-01 23:15:40

by Ivan Gorinov

Subject: [PATCH v3 2/3] x86: devicetree: enable multiprocessing in DT

Adding code to register the processors described in Device Tree.
APIC ID is specified in 'intel-apic_id' propery as used in U-Boot.
First address specified in 'reg' is used as default APIC ID.

Signed-off-by: Ivan Gorinov <[email protected]>
---
arch/x86/kernel/devicetree.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 44189ee..ef1cd85 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -130,6 +130,29 @@ static void __init dtb_setup_hpet(void)
#endif
}

+static void __init dtb_cpu_setup(void)
+{
+ struct device_node *dn;
+ struct resource r;
+ const void *prop;
+ int apic_id, version;
+ int ret;
+
+ version = GET_APIC_VERSION(apic_read(APIC_LVR));
+ for_each_node_by_type(dn, "cpu") {
+ prop = of_get_property(dn, "intel,apic-id", NULL);
+ if (prop) {
+ apic_id = be32_to_cpup(prop);
+ } else {
+ ret = of_address_to_resource(dn, 0, &r);
+ if (WARN_ON_ONCE(ret))
+ continue;
+ apic_id = r.start;
+ }
+ generic_processor_info(apic_id, version);
+ }
+}
+
static void __init dtb_lapic_setup(void)
{
#ifdef CONFIG_X86_LOCAL_APIC
@@ -153,8 +176,6 @@ static void __init dtb_lapic_setup(void)
smp_found_config = 1;
pic_mode = 1;
register_lapic_address(r.start);
- generic_processor_info(boot_cpu_physical_apicid,
- GET_APIC_VERSION(apic_read(APIC_LVR)));
#endif
}

@@ -256,6 +277,7 @@ static void __init dtb_ioapic_setup(void) {}
static void __init dtb_apic_setup(void)
{
dtb_lapic_setup();
+ dtb_cpu_setup();
dtb_ioapic_setup();
}

--
2.7.4

2018-03-02 14:54:33

by Thomas Gleixner

Subject: Re: [PATCH v3 2/3] x86: devicetree: enable multiprocessing in DT

On Thu, 1 Mar 2018, Ivan Gorinov wrote:

The patch order is wrong. Fixes first and then features simply because you
cannot test the feature patch standalone as the fix (3/3) is missing ....

> Adding code to register the processors described in Device Tree.

'Adding code...' is a pointless filler phrase.

> APIC ID is specified in 'intel-apic_id' propery as used in U-Boot.

s/propery/property/

Also this has nothing to do with U-Boot. The DT properties are described in
the DT bindings and not specified in U-Boot.

Where is the matching devicetree binding documented? This should be a
separate patch and needs to go into Documentation/devicetree/x86/

Please Cc the DT maintainers as we need their ack for that.

> First address specified in 'reg' is used as default APIC ID.

Changelogs should describe the context/problem and the approach how this is
fixed or made working. Something like this:

The current x86 device tree implementation does not support SMP.

Use the new DT bindings for describing CPUs and their APIC resources.

Hmm?

> Signed-off-by: Ivan Gorinov <[email protected]>
> ---
> arch/x86/kernel/devicetree.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index 44189ee..ef1cd85 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -130,6 +130,29 @@ static void __init dtb_setup_hpet(void)
> #endif
> }
>
> +static void __init dtb_cpu_setup(void)
> +{
> + struct device_node *dn;
> + struct resource r;
> + const void *prop;
> + int apic_id, version;
> + int ret;
> +
> + version = GET_APIC_VERSION(apic_read(APIC_LVR));
> + for_each_node_by_type(dn, "cpu") {
> + prop = of_get_property(dn, "intel,apic-id", NULL);
> + if (prop) {
> + apic_id = be32_to_cpup(prop);
> + } else {
> + ret = of_address_to_resource(dn, 0, &r);
> + if (WARN_ON_ONCE(ret))
> + continue;
> + apic_id = r.start;
> + }

What kind of logic is this? If a CPU node does not have apic id property
then it's invalid. That else clause is just voodoo programming at least
without a proper comment explaining it.

Thanks,

tglx