[U-Boot] [PATCH 4/9] x86: Move MP initialization codes into a common place

[Simon Glass]

Hi Bin,

On 11 June 2015 at 20:07, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jun 12, 2015 at 7:39 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 9 June 2015 at 01:45, Bin Meng <bmeng.cn at 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 at 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