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

[Bin Meng]

Hi Simon,

On Sat, Jun 13, 2015 at 3:10 AM, Simon Glass <sjg at chromium.org> wrote:
> 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.

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