[PATCH v6 0/2] x86/devicetree: Enable multiprocessing

[PATCH v6 0/2] x86/devicetree: Enable multiprocessing

[Ivan Gorinov]

Current x86 implementation of Device Tree does not support multiprocessing,
and the bindings documentation describes the "reg" property as CPU number
instead of hardware-assigned local APIC ID.

v6:
 * Calling of_property_read_u32() to get Local APIC ID from "reg"

 * DT documentation changes: corrected CPU node example and changed
   the "reg" property description

v5:
 * Using the "reg" property to specify Local APIC ID

Ivan Gorinov (2):
  of: Documentation: Specify local APIC ID in "reg"
  x86/devicetree: Use CPU description from Device Tree

 Documentation/devicetree/bindings/x86/ce4100.txt | 37 ++++++++++++++++------
 arch/x86/kernel/devicetree.c                     | 39 +++++++++++++++++-------
 2 files changed, 56 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH v6 1/2] of: Documentation: Specify local APIC ID in "reg"

[Ivan Gorinov]

Use the "reg" property to specify the processor's local APIC ID.
Local APIC ID is assigned by hardware and may differ from CPU number.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
---
 Documentation/devicetree/bindings/x86/ce4100.txt | 37 ++++++++++++++++++------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
index b49ae59..1c41cbd 100644
--- a/Documentation/devicetree/bindings/x86/ce4100.txt
+++ b/Documentation/devicetree/bindings/x86/ce4100.txt
@@ -7,17 +7,36 @@ Many of the "generic" devices like HPET or IO APIC have the ce4100
 name in their compatible property because they first appeared in this
 SoC.
 
-The CPU node
-------------
-	cpu@0 {
-		device_type = "cpu";
-		compatible = "intel,ce4100";
-		reg = <0>;
-		lapic = <&lapic0>;
+The CPU nodes
+-------------
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0x00 {
+			device_type = "cpu";
+			compatible = "intel,ce4100";
+			reg = <0x00>;
+		};
+
+		cpu@0x02 {
+			device_type = "cpu";
+			compatible = "intel,ce4100";
+			reg = <0x02>;
+		};
 	};
 
-The reg property describes the CPU number. The lapic property points to
-the local APIC timer.
+A "cpu" node describes one logical processor (hardware thread).
+
+Required properties:
+
+- device_type
+	Device type, must be "cpu".
+
+- reg
+	Local APIC ID, the unique number assigned to each processor by
+	system hardware.
 
 The SoC node
 ------------
-- 
2.7.4

[PATCH v6 2/2] x86/devicetree: Use CPU description from Device Tree

[Ivan Gorinov]

Current x86 Device Tree implementation does not support multiprocessing.
Use new DT bindings to describe the processors.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
---
 arch/x86/kernel/devicetree.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd387f..a601f08 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -131,31 +131,46 @@ static void __init dtb_setup_hpet(void)
 #endif
 }
 
+static void __init dtb_cpu_setup(void)
+{
+	struct device_node *dn;
+	u32 apic_id, version;
+	int ret;
+
+	version = GET_APIC_VERSION(apic_read(APIC_LVR));
+	for_each_node_by_type(dn, "cpu") {
+		ret = of_property_read_u32(dn, "reg", &apic_id);
+		if (ret < 0) {
+			pr_warn("%pOF: missing local APIC ID\n", dn);
+			continue;
+		}
+		generic_processor_info(apic_id, version);
+	}
+}
+
 static void __init dtb_lapic_setup(void)
 {
 #ifdef CONFIG_X86_LOCAL_APIC
 	struct device_node *dn;
 	struct resource r;
+	unsigned long lapic_addr = APIC_DEFAULT_PHYS_BASE;
 	int ret;
 
 	dn = of_find_compatible_node(NULL, NULL, "intel,ce4100-lapic");
-	if (!dn)
-		return;
-
-	ret = of_address_to_resource(dn, 0, &r);
-	if (WARN_ON(ret))
-		return;
+	if (dn) {
+		ret = of_address_to_resource(dn, 0, &r);
+		if (WARN_ON(ret))
+			return;
+		lapic_addr = r.start;
+	}
 
 	/* Did the boot loader setup the local APIC ? */
 	if (!boot_cpu_has(X86_FEATURE_APIC)) {
-		if (apic_force_enable(r.start))
+		if (apic_force_enable(lapic_addr))
 			return;
 	}
-	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)));
+	register_lapic_address(lapic_addr);
 #endif
 }
 
@@ -260,6 +275,7 @@ static void __init dtb_ioapic_setup(void) {}
 static void __init dtb_apic_setup(void)
 {
 	dtb_lapic_setup();
+	dtb_cpu_setup();
 	dtb_ioapic_setup();
 }
 
@@ -297,6 +313,7 @@ void __init x86_dtb_init(void)
 	if (!of_have_populated_dt())
 		return;
 
+	smp_found_config = 1;
 	dtb_setup_hpet();
 	dtb_apic_setup();
 }
-- 
2.7.4

Re: [PATCH v6 1/2] of: Documentation: Specify local APIC ID in "reg"

[Thomas Gleixner]

Rob,

On Tue, 13 Mar 2018, Ivan Gorinov wrote:

> Use the "reg" property to specify the processor's local APIC ID.
> Local APIC ID is assigned by hardware and may differ from CPU number.

Any opinion on this version?

Thanks,

	tglx

> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---
>  Documentation/devicetree/bindings/x86/ce4100.txt | 37 ++++++++++++++++++------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
> index b49ae59..1c41cbd 100644
> --- a/Documentation/devicetree/bindings/x86/ce4100.txt
> +++ b/Documentation/devicetree/bindings/x86/ce4100.txt
> @@ -7,17 +7,36 @@ Many of the "generic" devices like HPET or IO APIC have the ce4100
>  name in their compatible property because they first appeared in this
>  SoC.
>  
> -The CPU node
> -------------
> -	cpu@0 {
> -		device_type = "cpu";
> -		compatible = "intel,ce4100";
> -		reg = <0>;
> -		lapic = <&lapic0>;
> +The CPU nodes
> +-------------
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0x00 {
> +			device_type = "cpu";
> +			compatible = "intel,ce4100";
> +			reg = <0x00>;
> +		};
> +
> +		cpu@0x02 {
> +			device_type = "cpu";
> +			compatible = "intel,ce4100";
> +			reg = <0x02>;
> +		};
>  	};
>  
> -The reg property describes the CPU number. The lapic property points to
> -the local APIC timer.
> +A "cpu" node describes one logical processor (hardware thread).
> +
> +Required properties:
> +
> +- device_type
> +	Device type, must be "cpu".
> +
> +- reg
> +	Local APIC ID, the unique number assigned to each processor by
> +	system hardware.
>  
>  The SoC node
>  ------------
> -- 
> 2.7.4
> 
> 

Re: [PATCH v6 2/2] x86/devicetree: Use CPU description from Device Tree

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

Hi Ivan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ivan-Gorinov/x86-devicetree-Enable-multiprocessing/20180316-052522
config: i386-randconfig-sb0-03200602 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kernel/devicetree.c: In function 'x86_dtb_init':
>> arch/x86/kernel/devicetree.c:311:19: error: lvalue required as left operand of assignment
     smp_found_config = 1;
                      ^

vim +311 arch/x86/kernel/devicetree.c

   303	
   304	void __init x86_dtb_init(void)
   305	{
   306		x86_flattree_get_config();
   307	
   308		if (!of_have_populated_dt())
   309			return;
   310	
 > 311		smp_found_config = 1;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28672 bytes --]

Re: [PATCH v6 1/2] of: Documentation: Specify local APIC ID in "reg"

[Rob Herring]

On Tue, Mar 13, 2018 at 5:05 PM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Use the "reg" property to specify the processor's local APIC ID.
> Local APIC ID is assigned by hardware and may differ from CPU number.

Is "CPU number" a s/w visible h/w number or has it just been an index
for DT? In the latter case, I'm okay with this change. In the former
case, you should stick to the existing numbering. For example on ARM,
the number here corresponds to a core ID number in a register called
MPIDR.

>
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---
>  Documentation/devicetree/bindings/x86/ce4100.txt | 37 ++++++++++++++++++------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
> index b49ae59..1c41cbd 100644
> --- a/Documentation/devicetree/bindings/x86/ce4100.txt
> +++ b/Documentation/devicetree/bindings/x86/ce4100.txt
> @@ -7,17 +7,36 @@ Many of the "generic" devices like HPET or IO APIC have the ce4100
>  name in their compatible property because they first appeared in this
>  SoC.
>
> -The CPU node
> -------------
> -       cpu@0 {
> -               device_type = "cpu";
> -               compatible = "intel,ce4100";
> -               reg = <0>;
> -               lapic = <&lapic0>;
> +The CPU nodes
> +-------------
> +
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0x00 {

Drop the '0x' and leading 0s.

> +                       device_type = "cpu";
> +                       compatible = "intel,ce4100";
> +                       reg = <0x00>;
> +               };
> +
> +               cpu@0x02 {
> +                       device_type = "cpu";
> +                       compatible = "intel,ce4100";
> +                       reg = <0x02>;
> +               };
>         };
>
> -The reg property describes the CPU number. The lapic property points to
> -the local APIC timer.
> +A "cpu" node describes one logical processor (hardware thread).
> +
> +Required properties:
> +
> +- device_type
> +       Device type, must be "cpu".
> +
> +- reg
> +       Local APIC ID, the unique number assigned to each processor by
> +       system hardware.
>
>  The SoC node
>  ------------
> --
> 2.7.4
>

Re: [PATCH v6 1/2] of: Documentation: Specify local APIC ID in "reg"

[Ivan Gorinov]

On Mon, Mar 19, 2018 at 07:39:52PM -0500, Rob Herring wrote:
> On Tue, Mar 13, 2018 at 5:05 PM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> > Use the "reg" property to specify the processor's local APIC ID.
> > Local APIC ID is assigned by hardware and may differ from CPU number.
> 
> Is "CPU number" a s/w visible h/w number or has it just been an index
> for DT? In the latter case, I'm okay with this change. In the former
> case, you should stick to the existing numbering. For example on ARM,
> the number here corresponds to a core ID number in a register called
> MPIDR.

The latter case. Apparently, "CPU number" was just an index in the list.
Local APIC ID is the s/w visible h/w assigned number.
Some processor models allow local APIC ID to be changed by software, but
CPUID instruction executed with %eax = 0x0b always returns the initial ID
assigned by hardware in %edx.

APIC ID does not match index in the list in many systems.

> >
> > Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> > ---
> >  Documentation/devicetree/bindings/x86/ce4100.txt | 37 ++++++++++++++++++------
> >  1 file changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
> > index b49ae59..1c41cbd 100644
> > --- a/Documentation/devicetree/bindings/x86/ce4100.txt
> > +++ b/Documentation/devicetree/bindings/x86/ce4100.txt
> > @@ -7,17 +7,36 @@ Many of the "generic" devices like HPET or IO APIC have the ce4100
> >  name in their compatible property because they first appeared in this
> >  SoC.
> >
> > -The CPU node
> > -------------
> > -       cpu@0 {
> > -               device_type = "cpu";
> > -               compatible = "intel,ce4100";
> > -               reg = <0>;
> > -               lapic = <&lapic0>;
> > +The CPU nodes
> > +-------------
> > +
> > +       cpus {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               cpu@0x00 {
> 
> Drop the '0x' and leading 0s.
> 
> > +                       device_type = "cpu";
> > +                       compatible = "intel,ce4100";
> > +                       reg = <0x00>;
> > +               };
> > +
> > +               cpu@0x02 {
> > +                       device_type = "cpu";
> > +                       compatible = "intel,ce4100";
> > +                       reg = <0x02>;
> > +               };
> >         };
> >
> > -The reg property describes the CPU number. The lapic property points to
> > -the local APIC timer.
> > +A "cpu" node describes one logical processor (hardware thread).
> > +
> > +Required properties:
> > +
> > +- device_type
> > +       Device type, must be "cpu".
> > +
> > +- reg
> > +       Local APIC ID, the unique number assigned to each processor by
> > +       system hardware.
> >
> >  The SoC node
> >  ------------
> > --
> > 2.7.4
> >

Re: [PATCH v6 1/2] of: Documentation: Specify local APIC ID in "reg"

[Thomas Gleixner]

On Tue, 20 Mar 2018, Ivan Gorinov wrote:

> On Mon, Mar 19, 2018 at 07:39:52PM -0500, Rob Herring wrote:
> > On Tue, Mar 13, 2018 at 5:05 PM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> > > Use the "reg" property to specify the processor's local APIC ID.
> > > Local APIC ID is assigned by hardware and may differ from CPU number.
> > 
> > Is "CPU number" a s/w visible h/w number or has it just been an index
> > for DT? In the latter case, I'm okay with this change. In the former
> > case, you should stick to the existing numbering. For example on ARM,
> > the number here corresponds to a core ID number in a register called
> > MPIDR.
> 
> The latter case. Apparently, "CPU number" was just an index in the list.
> Local APIC ID is the s/w visible h/w assigned number.
> Some processor models allow local APIC ID to be changed by software, but
> CPUID instruction executed with %eax = 0x0b always returns the initial ID
> assigned by hardware in %edx.
> 
> APIC ID does not match index in the list in many systems.

Please document that in the changelog.

Thanks,

	tglx