diff mbox

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

Message ID BLU437-SMTP338713357EB6ABB95196DBFBE0@phx.gbl
State Superseded
Delegated to: Simon Glass
Headers show

Commit Message

Bin Meng June 9, 2015, 7:45 a.m. UTC
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(-)

Comments

Simon Glass June 11, 2015, 11:39 p.m. UTC | #1
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
Bin Meng June 12, 2015, 2:07 a.m. UTC | #2
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
Simon Glass June 12, 2015, 7:10 p.m. UTC | #3
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
Bin Meng June 13, 2015, 7:34 a.m. UTC | #4
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
diff mbox

Patch

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;
 }