Hi Simon,
On Wed, Apr 29, 2015 at 10:00 PM, Simon Glass <s...@chromium.org> wrote:
> Hi Bin,
>
> On 29 April 2015 at 07:57, Bin Meng <bmeng...@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Apr 29, 2015 at 10:25 AM, Simon Glass <s...@chromium.org> wrote:
>>> This driver supports multi-core init and sets up the CPU frequencies
>>> correctly.
>>>
>>> Signed-off-by: Simon Glass <s...@chromium.org>
>>> ---
>>>
>>> Changes in v2: None
>>>
>>> arch/x86/cpu/baytrail/Makefile | 1 +
>>> arch/x86/cpu/baytrail/cpu.c | 206
>>> +++++++++++++++++++++++++++++++
>>> arch/x86/include/asm/arch-baytrail/msr.h | 30 +++++
>>> 3 files changed, 237 insertions(+)
>>> create mode 100644 arch/x86/cpu/baytrail/cpu.c
>>> create mode 100644 arch/x86/include/asm/arch-baytrail/msr.h
>>>
>>> diff --git a/arch/x86/cpu/baytrail/Makefile b/arch/x86/cpu/baytrail/Makefile
>>> index 8914e8b..c78b644 100644
>>> --- a/arch/x86/cpu/baytrail/Makefile
>>> +++ b/arch/x86/cpu/baytrail/Makefile
>>> @@ -4,6 +4,7 @@
>>> # SPDX-License-Identifier: GPL-2.0+
>>> #
>>>
>>> +obj-y += cpu.o
>>> obj-y += early_uart.o
>>> obj-y += fsp_configs.o
>>> obj-y += pci.o
>>> diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c
>>> new file mode 100644
>>> index 0000000..5a2a8ee
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/baytrail/cpu.c
>>> @@ -0,0 +1,206 @@
>>> +/*
>>> + * Copyright (C) 2015 Google, Inc
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + *
>>> + * Based on code from coreboot
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <cpu.h>
>>> +#include <dm.h>
>>> +#include <asm/cpu.h>
>>> +#include <asm/lapic.h>
>>> +#include <asm/mp.h>
>>> +#include <asm/msr.h>
>>> +#include <asm/turbo.h>
>>> +#include <asm/arch/msr.h>
>>> +
>>> +#ifdef CONFIG_SMP
>>> +static int enable_smis(struct udevice *cpu, void *unused)
>>> +{
>>> + return 0;
>>> +}
>>
>> What is this function for? Is this a must-have?
>
> It's partly a placeholder, and also is intended to ensure that the APs
> are all started before the main CPU continues execution.
>
>>
>>> +
>>> +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 ecx = 0;
>>> +
>>> + /*
>>> + * Use the algorithm described in Intel 64 and IA-32 Architectures
>>> + * Software Developer's Manual Volume 3 (3A, 3B & 3C): System
>>> + * Programming Guide, Jan-2015. Section 8.9.2: Hierarchical Mapping
>>> + * of CPUID Extended Topology Leaf.
>>> + */
>>> + while (1) {
>>> + struct cpuid_result leaf_b;
>>> +
>>> + leaf_b = cpuid_ext(0xb, ecx);
>>> +
>>> + /*
>>> + * Bay Trail doesn't have hyperthreading so just determine
>>> the
>>> + * number of cores by from level type (ecx[15:8] == * 2)
>>> + */
>>> + if ((leaf_b.ecx & 0xff00) == 0x0200)
>>> + return leaf_b.ebx & 0xffff;
>>> + ecx++;
>>> + }
>>> +}
>>
>> Since we already describe all cpus in the device tree, is this dynamic
>> probe really needed?
>
> With MinnowMax I'd like to support the single-core version of the
> board also. It could have its own device tree, but I don't want to
> break in this case. However, this case is not tested.
Do you mean that there is a specific version of MinnowMax board which
contains an single core version of Atom E3800 (?, maybe another brand
name)? But as you said, we can create another device tree for the
single core version. No? Or maybe we fix up the DTB node here
dynamically, that we still only have one device tree to describe the
dual-core version, but after this dynamic probe, we fix up the DTB to
remove one cpu node if we get a single core version?
>>
>>> +
>>> +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;
>>> +}
>>> +
>>> +void set_max_freq(void)
>>
>> Should this be static?
>
> Yes
>
>>
>>> +{
>>> + msr_t perf_ctl;
>>> + msr_t msr;
>>> +
>>> + /* Enable speed step */
>>> + msr = msr_read(MSR_IA32_MISC_ENABLES);
>>> + msr.lo |= (1 << 16);
>>> + msr_write(MSR_IA32_MISC_ENABLES, msr);
>>> +
>>> + /*
>>> + * Set guaranteed ratio [21:16] from IACORE_RATIOS to bits [15:8] of
>>> + * the PERF_CTL
>>> + */
>>> + msr = msr_read(MSR_IACORE_RATIOS);
>>> + perf_ctl.lo = (msr.lo & 0x3f0000) >> 8;
>>> +
>>> + /*
>>> + * Set guaranteed vid [21:16] from IACORE_VIDS to bits [7:0] of
>>> + * the PERF_CTL
>>> + */
>>> + msr = msr_read(MSR_IACORE_VIDS);
>>> + perf_ctl.lo |= (msr.lo & 0x7f0000) >> 16;
>>> + perf_ctl.hi = 0;
>>> +
>>> + msr_write(MSR_IA32_PERF_CTL, perf_ctl);
>>> +}
>>> +
>>> +static int cpu_x86_baytrail_probe(struct udevice *dev)
>>> +{
>>> + debug("Init baytrail core\n");
>>
>> BayTrail?
>
> OK
>
>>
>>> +
>>> + /*
>>> + * On bay trail the turbo disable bit is actually scoped at the
>>
>> BayTrail?
>>
>>> + * building-block level, not package. For non-BSP cores that are
>>> + * within a building block, enable turbo. The cores within the BSP's
>>> + * building block will just see it already enabled and move on.
>>> + */
>>> + if (lapicid())
>>> + turbo_enable();
>>> +
>>> + /* Dynamic L2 shrink enable and threshold */
>>> + msr_clrsetbits_64(MSR_PMG_CST_CONFIG_CONTROL, 0x3f000f, 0xe0008),
>>> +
>>> + /* Disable C1E */
>>> + msr_clrsetbits_64(MSR_POWER_CTL, 2, 0);
>>> + msr_setbits_64(MSR_POWER_MISC, 0x44);
>>> +
>>> + /* Set this core to max frequency ratio */
>>> + set_max_freq();
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static unsigned bus_freq(void)
>>> +{
>>> + msr_t clk_info = msr_read(MSR_BSEL_CR_OVERCLOCK_CONTROL);
>>> + switch (clk_info.lo & 0x3) {
>>> + case 0:
>>> + return 83333333;
>>> + case 1:
>>> + return 100000000;
>>> + case 2:
>>> + return 133333333;
>>> + case 3:
>>> + return 116666666;
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static unsigned long tsc_freq(void)
>>> +{
>>> + msr_t platform_info;
>>> + ulong bclk = bus_freq();
>>> +
>>> + if (!bclk)
>>> + return 0;
>>> +
>>> + platform_info = msr_read(MSR_PLATFORM_INFO);
>>> +
>>> + return bclk * ((platform_info.lo >> 8) & 0xff);
>>> +}
>>> +
>>> +static int baytrail_get_info(struct udevice *dev, struct cpu_info *info)
>>> +{
>>> + info->cpu_freq = tsc_freq();
>>> + info->features = 1 << CPU_FEAT_L1_CACHE | 1 << CPU_FEAT_MMU;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int cpu_x86_baytrail_bind(struct udevice *dev)
>>> +{
>>> + struct cpu_platdata *plat = dev_get_parent_platdata(dev);
>>> +
>>> + plat->cpu_id = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>>> + "intel,apic-id", -1);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct cpu_ops cpu_x86_baytrail_ops = {
>>> + .get_desc = x86_cpu_get_desc,
>>> + .get_info = baytrail_get_info,
>>> +};
>>> +
>>> +static const struct udevice_id cpu_x86_baytrail_ids[] = {
>>> + { .compatible = "intel,baytrail-cpu" },
>>> + { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(cpu_x86_baytrail_drv) = {
>>> + .name = "cpu_x86_baytrail",
>>> + .id = UCLASS_CPU,
>>> + .of_match = cpu_x86_baytrail_ids,
>>> + .bind = cpu_x86_baytrail_bind,
>>> + .probe = cpu_x86_baytrail_probe,
>>> + .ops = &cpu_x86_baytrail_ops,
>>> +};
>>> diff --git a/arch/x86/include/asm/arch-baytrail/msr.h
>>> b/arch/x86/include/asm/arch-baytrail/msr.h
>>> new file mode 100644
>>> index 0000000..1975aec
>>> --- /dev/null
>>> +++ b/arch/x86/include/asm/arch-baytrail/msr.h
>>> @@ -0,0 +1,30 @@
>>> +/*
>>> + * Copyright (C) 2015 Google, Inc
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + */
>>> +
>>> +#ifndef __asm_arch_msr_h
>>> +#define __asm_arch_msr_h
>>
>> Should be capital letters, or (see below)
>>
>>> +
>>> +#define MSR_BSEL_CR_OVERCLOCK_CONTROL 0xcd
>>> +#define MSR_PMG_CST_CONFIG_CONTROL 0xe2
>>> +#define SINGLE_PCTL (1 << 11)
>>> +#define MSR_POWER_MISC 0x120
>>> +#define ENABLE_ULFM_AUTOCM_MASK (1 << 2)
>>> +#define ENABLE_INDP_AUTOCM_MASK (1 << 3)
>>> +#define MSR_IA32_MISC_ENABLES 0x1a0
>>> +#define MSR_POWER_CTL 0x1fc
>>> +#define MSR_PKG_POWER_SKU_UNIT 0x606
>>> +#define MSR_IACORE_RATIOS 0x66a
>>> +#define MSR_IACORE_TURBO_RATIOS 0x66c
>>> +#define MSR_IACORE_VIDS 0x66b
>>> +#define MSR_IACORE_TURBO_VIDS 0x66d
>>> +#define MSR_PKG_TURBO_CFG1 0x670
>>> +#define MSR_CPU_TURBO_WKLD_CFG1 0x671
>>> +#define MSR_CPU_TURBO_WKLD_CFG2 0x672
>>> +#define MSR_CPU_THERM_CFG1 0x673
>>> +#define MSR_CPU_THERM_CFG2 0x674
>>> +#define MSR_CPU_THERM_SENS_CFG 0x675
>>> +
>>
>> Should these be all put into arch/x86/include/asm/msr-index.h, a
>> single place for all x86 processors' MSR?
>
> I was worried that they might be specific to this CPU. But if they are
> common then yes they should go in the common file.
>
Maybe some of them are BayTrail-specific. But there MSRs are
documented in the Intel 64 and IA-32 Architectures Software
Developer's Manual Volume 3, right? If yes, I think it's fine to put
them at just one place.
Regards,
Bin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
