[U-Boot] [PATCH 4/9] x86: Move MP initialization codes into a common place
Most of the MP initialization codes in arch/x86/cpu/baytrail/cpu.c is common to all x86 processors, except detect_num_cpus() which varies from cpu to cpu. Move these to arch/x86/cpu/cpu.c and declare a weak detect_num_cpus() which just returns 2 which is minimally required.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/cpu/baytrail/cpu.c | 44 +----------------------------------------- arch/x86/cpu/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c index 05156a5..7805056 100644 --- a/arch/x86/cpu/baytrail/cpu.c +++ b/arch/x86/cpu/baytrail/cpu.c @@ -12,23 +12,11 @@ #include <asm/cpu.h> #include <asm/cpu_x86.h> #include <asm/lapic.h> -#include <asm/mp.h> #include <asm/msr.h> #include <asm/turbo.h>
#ifdef CONFIG_SMP -static int enable_smis(struct udevice *cpu, void *unused) -{ - return 0; -} - -static struct mp_flight_record mp_steps[] = { - MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL), - /* Wait for APs to finish initialization before proceeding. */ - MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL), -}; - -static int detect_num_cpus(void) +int detect_num_cpus(void) { int ecx = 0;
@@ -52,38 +40,8 @@ static int detect_num_cpus(void) ecx++; } } - -static int baytrail_init_cpus(void) -{ - struct mp_params mp_params; - - lapic_setup(); - - mp_params.num_cpus = detect_num_cpus(); - mp_params.parallel_microcode_load = 0, - mp_params.flight_plan = &mp_steps[0]; - mp_params.num_records = ARRAY_SIZE(mp_steps); - mp_params.microcode_pointer = 0; - - if (mp_init(&mp_params)) { - printf("Warning: MP init failure\n"); - return -EIO; - } - - return 0; -} #endif
-int x86_init_cpus(void) -{ -#ifdef CONFIG_SMP - debug("Init additional CPUs\n"); - baytrail_init_cpus(); -#endif - - return 0; -} - static void set_max_freq(void) { msr_t perf_ctl; diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index ffb6e43..ddc7dc3 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -21,10 +21,13 @@
#include <common.h> #include <command.h> +#include <dm.h> #include <errno.h> #include <malloc.h> #include <asm/control_regs.h> #include <asm/cpu.h> +#include <asm/lapic.h> +#include <asm/mp.h> #include <asm/post.h> #include <asm/processor.h> #include <asm/processor-flags.h> @@ -601,8 +604,52 @@ int last_stage_init(void) } #endif
+#ifdef CONFIG_SMP +static int enable_smis(struct udevice *cpu, void *unused) +{ + return 0; +} + +static struct mp_flight_record mp_steps[] = { + MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL), + /* Wait for APs to finish initialization before proceeding */ + MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL), +}; + +__weak int detect_num_cpus(void) +{ + /* We need at least 2 cores to perform mp_init() */ + return 2; +} + +static int x86_mp_init(void) +{ + struct mp_params mp_params; + + lapic_setup(); + + mp_params.num_cpus = detect_num_cpus(); + mp_params.parallel_microcode_load = 0, + mp_params.flight_plan = &mp_steps[0]; + mp_params.num_records = ARRAY_SIZE(mp_steps); + mp_params.microcode_pointer = 0; + + if (mp_init(&mp_params)) { + printf("Warning: MP init failure\n"); + return -EIO; + } + + return 0; +} +#endif + __weak int x86_init_cpus(void) { +#ifdef CONFIG_SMP + debug("Init additional CPUs\n"); + x86_mp_init(); +#endif + return 0; }
Hi Bin,
On 9 June 2015 at 01:45, Bin Meng bmeng.cn@gmail.com wrote:
Most of the MP initialization codes in arch/x86/cpu/baytrail/cpu.c is common to all x86 processors, except detect_num_cpus() which varies from cpu to cpu. Move these to arch/x86/cpu/cpu.c and declare a weak detect_num_cpus() which just returns 2 which is minimally required.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/cpu.c | 44 +----------------------------------------- arch/x86/cpu/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c index 05156a5..7805056 100644 --- a/arch/x86/cpu/baytrail/cpu.c +++ b/arch/x86/cpu/baytrail/cpu.c @@ -12,23 +12,11 @@ #include <asm/cpu.h> #include <asm/cpu_x86.h> #include <asm/lapic.h> -#include <asm/mp.h> #include <asm/msr.h> #include <asm/turbo.h>
#ifdef CONFIG_SMP -static int enable_smis(struct udevice *cpu, void *unused) -{
return 0;-}
-static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding. */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),-};
-static int detect_num_cpus(void) +int detect_num_cpus(void) { int ecx = 0;
@@ -52,38 +40,8 @@ static int detect_num_cpus(void) ecx++; } }
-static int baytrail_init_cpus(void) -{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;-} #endif
-int x86_init_cpus(void) -{ -#ifdef CONFIG_SMP
debug("Init additional CPUs\n");baytrail_init_cpus();-#endif
return 0;-}
static void set_max_freq(void) { msr_t perf_ctl; diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index ffb6e43..ddc7dc3 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -21,10 +21,13 @@
#include <common.h> #include <command.h> +#include <dm.h> #include <errno.h> #include <malloc.h> #include <asm/control_regs.h> #include <asm/cpu.h> +#include <asm/lapic.h> +#include <asm/mp.h> #include <asm/post.h> #include <asm/processor.h> #include <asm/processor-flags.h> @@ -601,8 +604,52 @@ int last_stage_init(void) } #endif
+#ifdef CONFIG_SMP +static int enable_smis(struct udevice *cpu, void *unused) +{
return 0;+}
+static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),+};
+__weak int detect_num_cpus(void)
Does this need to be weak? We could perhaps require that the function exists?
+{
/* We need at least 2 cores to perform mp_init() */return 2;+}
+static int x86_mp_init(void) +{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;+} +#endif
__weak int x86_init_cpus(void) { +#ifdef CONFIG_SMP
debug("Init additional CPUs\n");x86_mp_init();+#endif
return 0;}
-- 1.8.2.1
Regards, Simon
Hi Simon,
On Fri, Jun 12, 2015 at 7:39 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 9 June 2015 at 01:45, Bin Meng bmeng.cn@gmail.com wrote:
Most of the MP initialization codes in arch/x86/cpu/baytrail/cpu.c is common to all x86 processors, except detect_num_cpus() which varies from cpu to cpu. Move these to arch/x86/cpu/cpu.c and declare a weak detect_num_cpus() which just returns 2 which is minimally required.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/cpu.c | 44 +----------------------------------------- arch/x86/cpu/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c index 05156a5..7805056 100644 --- a/arch/x86/cpu/baytrail/cpu.c +++ b/arch/x86/cpu/baytrail/cpu.c @@ -12,23 +12,11 @@ #include <asm/cpu.h> #include <asm/cpu_x86.h> #include <asm/lapic.h> -#include <asm/mp.h> #include <asm/msr.h> #include <asm/turbo.h>
#ifdef CONFIG_SMP -static int enable_smis(struct udevice *cpu, void *unused) -{
return 0;-}
-static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding. */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),-};
-static int detect_num_cpus(void) +int detect_num_cpus(void) { int ecx = 0;
@@ -52,38 +40,8 @@ static int detect_num_cpus(void) ecx++; } }
-static int baytrail_init_cpus(void) -{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;-} #endif
-int x86_init_cpus(void) -{ -#ifdef CONFIG_SMP
debug("Init additional CPUs\n");baytrail_init_cpus();-#endif
return 0;-}
static void set_max_freq(void) { msr_t perf_ctl; diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index ffb6e43..ddc7dc3 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -21,10 +21,13 @@
#include <common.h> #include <command.h> +#include <dm.h> #include <errno.h> #include <malloc.h> #include <asm/control_regs.h> #include <asm/cpu.h> +#include <asm/lapic.h> +#include <asm/mp.h> #include <asm/post.h> #include <asm/processor.h> #include <asm/processor-flags.h> @@ -601,8 +604,52 @@ int last_stage_init(void) } #endif
+#ifdef CONFIG_SMP +static int enable_smis(struct udevice *cpu, void *unused) +{
return 0;+}
+static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),+};
+__weak int detect_num_cpus(void)
Does this need to be weak? We could perhaps require that the function exists?
Yes, since I don't see there is a common way to detect number of cpu cores. Or maybe I am not aware of one. If we want to remove the weak, maybe we can just switch to count the number of cpu nodes in /cpus from device tree, which should be generic for all cases. I believe we discussed this during the review when you added MP stuff.
+{
/* We need at least 2 cores to perform mp_init() */return 2;+}
+static int x86_mp_init(void) +{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;+} +#endif
__weak int x86_init_cpus(void) { +#ifdef CONFIG_SMP
debug("Init additional CPUs\n");x86_mp_init();+#endif
return 0;}
-- 1.8.2.1
Regards, Simon
Regards, Bin
Hi Bin,
On 11 June 2015 at 20:07, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Fri, Jun 12, 2015 at 7:39 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 9 June 2015 at 01:45, Bin Meng bmeng.cn@gmail.com wrote:
Most of the MP initialization codes in arch/x86/cpu/baytrail/cpu.c is common to all x86 processors, except detect_num_cpus() which varies from cpu to cpu. Move these to arch/x86/cpu/cpu.c and declare a weak detect_num_cpus() which just returns 2 which is minimally required.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/cpu.c | 44 +----------------------------------------- arch/x86/cpu/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c index 05156a5..7805056 100644 --- a/arch/x86/cpu/baytrail/cpu.c +++ b/arch/x86/cpu/baytrail/cpu.c @@ -12,23 +12,11 @@ #include <asm/cpu.h> #include <asm/cpu_x86.h> #include <asm/lapic.h> -#include <asm/mp.h> #include <asm/msr.h> #include <asm/turbo.h>
#ifdef CONFIG_SMP -static int enable_smis(struct udevice *cpu, void *unused) -{
return 0;-}
-static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding. */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),-};
-static int detect_num_cpus(void) +int detect_num_cpus(void) { int ecx = 0;
@@ -52,38 +40,8 @@ static int detect_num_cpus(void) ecx++; } }
-static int baytrail_init_cpus(void) -{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;-} #endif
-int x86_init_cpus(void) -{ -#ifdef CONFIG_SMP
debug("Init additional CPUs\n");baytrail_init_cpus();-#endif
return 0;-}
static void set_max_freq(void) { msr_t perf_ctl; diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index ffb6e43..ddc7dc3 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -21,10 +21,13 @@
#include <common.h> #include <command.h> +#include <dm.h> #include <errno.h> #include <malloc.h> #include <asm/control_regs.h> #include <asm/cpu.h> +#include <asm/lapic.h> +#include <asm/mp.h> #include <asm/post.h> #include <asm/processor.h> #include <asm/processor-flags.h> @@ -601,8 +604,52 @@ int last_stage_init(void) } #endif
+#ifdef CONFIG_SMP +static int enable_smis(struct udevice *cpu, void *unused) +{
return 0;+}
+static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),+};
+__weak int detect_num_cpus(void)
Does this need to be weak? We could perhaps require that the function exists?
Yes, since I don't see there is a common way to detect number of cpu cores. Or maybe I am not aware of one. If we want to remove the weak, maybe we can just switch to count the number of cpu nodes in /cpus from device tree, which should be generic for all cases. I believe we discussed this during the review when you added MP stuff.
I worry what might happen if the device tree says 4 but there are only 2 CPUs. There should be a way to detect the number of CPUs gracefully in each arch.
Can you add get_cpu_count() as a method to the UCLASS_CPU ops perhaps? Then baytrail can use it.
mp_init() can make the get_cpu_count() call after finding the first CPU, and we can remove num_cpus from struct mp_params.
To me that seams cleaner than creating a weak function.
+{
/* We need at least 2 cores to perform mp_init() */return 2;+}
+static int x86_mp_init(void) +{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;+} +#endif
__weak int x86_init_cpus(void) { +#ifdef CONFIG_SMP
debug("Init additional CPUs\n");x86_mp_init();+#endif
return 0;}
-- 1.8.2.1
Regards, Simon
Hi Simon,
On Sat, Jun 13, 2015 at 3:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 11 June 2015 at 20:07, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Fri, Jun 12, 2015 at 7:39 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 9 June 2015 at 01:45, Bin Meng bmeng.cn@gmail.com wrote:
Most of the MP initialization codes in arch/x86/cpu/baytrail/cpu.c is common to all x86 processors, except detect_num_cpus() which varies from cpu to cpu. Move these to arch/x86/cpu/cpu.c and declare a weak detect_num_cpus() which just returns 2 which is minimally required.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/cpu.c | 44 +----------------------------------------- arch/x86/cpu/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c index 05156a5..7805056 100644 --- a/arch/x86/cpu/baytrail/cpu.c +++ b/arch/x86/cpu/baytrail/cpu.c @@ -12,23 +12,11 @@ #include <asm/cpu.h> #include <asm/cpu_x86.h> #include <asm/lapic.h> -#include <asm/mp.h> #include <asm/msr.h> #include <asm/turbo.h>
#ifdef CONFIG_SMP -static int enable_smis(struct udevice *cpu, void *unused) -{
return 0;-}
-static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding. */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),-};
-static int detect_num_cpus(void) +int detect_num_cpus(void) { int ecx = 0;
@@ -52,38 +40,8 @@ static int detect_num_cpus(void) ecx++; } }
-static int baytrail_init_cpus(void) -{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;-} #endif
-int x86_init_cpus(void) -{ -#ifdef CONFIG_SMP
debug("Init additional CPUs\n");baytrail_init_cpus();-#endif
return 0;-}
static void set_max_freq(void) { msr_t perf_ctl; diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index ffb6e43..ddc7dc3 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -21,10 +21,13 @@
#include <common.h> #include <command.h> +#include <dm.h> #include <errno.h> #include <malloc.h> #include <asm/control_regs.h> #include <asm/cpu.h> +#include <asm/lapic.h> +#include <asm/mp.h> #include <asm/post.h> #include <asm/processor.h> #include <asm/processor-flags.h> @@ -601,8 +604,52 @@ int last_stage_init(void) } #endif
+#ifdef CONFIG_SMP +static int enable_smis(struct udevice *cpu, void *unused) +{
return 0;+}
+static struct mp_flight_record mp_steps[] = {
MP_FR_BLOCK_APS(mp_init_cpu, NULL, mp_init_cpu, NULL),/* Wait for APs to finish initialization before proceeding */MP_FR_BLOCK_APS(NULL, NULL, enable_smis, NULL),+};
+__weak int detect_num_cpus(void)
Does this need to be weak? We could perhaps require that the function exists?
Yes, since I don't see there is a common way to detect number of cpu cores. Or maybe I am not aware of one. If we want to remove the weak, maybe we can just switch to count the number of cpu nodes in /cpus from device tree, which should be generic for all cases. I believe we discussed this during the review when you added MP stuff.
I worry what might happen if the device tree says 4 but there are only 2 CPUs. There should be a way to detect the number of CPUs gracefully in each arch.
I don't think there will be any problem for device tree says 4 but actually 2. U-Boot will just enable 2 CPUs and report 2 to the OS. The case that might have a problem is that the device tree says 2 but there are actually 4 CPUs. There is a check_cpu_devices() call in mp_init() which will put a debug message "Warning: Device tree does not describe all CPUs. Extra ones will not be started correctly". There are some places (like SFI) in U-Boot that look up the cpus node in device tree , and we will end up CPU#2~3 not reported to OS. But why should we create a device tree which does not describe the hardware in the first place?
Can you add get_cpu_count() as a method to the UCLASS_CPU ops perhaps? Then baytrail can use it.
mp_init() can make the get_cpu_count() call after finding the first CPU, and we can remove num_cpus from struct mp_params.
To me that seams cleaner than creating a weak function.
Yes, I can try to implement that.
+{
/* We need at least 2 cores to perform mp_init() */return 2;+}
+static int x86_mp_init(void) +{
struct mp_params mp_params;lapic_setup();mp_params.num_cpus = detect_num_cpus();mp_params.parallel_microcode_load = 0,mp_params.flight_plan = &mp_steps[0];mp_params.num_records = ARRAY_SIZE(mp_steps);mp_params.microcode_pointer = 0;if (mp_init(&mp_params)) {printf("Warning: MP init failure\n");return -EIO;}return 0;+} +#endif
__weak int x86_init_cpus(void) { +#ifdef CONFIG_SMP
debug("Init additional CPUs\n");x86_mp_init();+#endif
return 0;}
--
Regards, Bin
participants (2)
-
Bin Meng -
Simon Glass