[U-Boot] [PATCH] x86: queensbay: switche P state to the highest freq

[Christian Gmeiner]

Hi Bin

Thanks for you quick review!

>
> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner
> <christian.gmeiner at gmail.com> wrote:
>> Fixes performance issue when running vx5/vx7 images.
>
> Could you elaborate more on what performance issue you are seeing?
>

The cache performance was bad without this change. We found this problem
when switching from bldk to u-boot.

old: https://imagebin.ca/v/3xssw7stbY6A
new: https://imagebin.ca/v/3xstHMCC9fmJ

> nits: better to spell out "VxWorks"

ok

>
>>
>> Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
>> ---
>>  arch/x86/cpu/queensbay/Makefile |  2 +-
>>  arch/x86/cpu/queensbay/cpu.c    | 61 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 62 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/x86/cpu/queensbay/cpu.c
>>
>> diff --git a/arch/x86/cpu/queensbay/Makefile b/arch/x86/cpu/queensbay/Makefile
>> index c0681995bd..3dd23465d4 100644
>> --- a/arch/x86/cpu/queensbay/Makefile
>> +++ b/arch/x86/cpu/queensbay/Makefile
>> @@ -5,4 +5,4 @@
>>  #
>>
>>  obj-y += fsp_configs.o irq.o
>> -obj-y += tnc.o
>> +obj-y += tnc.o cpu.o
>> diff --git a/arch/x86/cpu/queensbay/cpu.c b/arch/x86/cpu/queensbay/cpu.c
>> new file mode 100644
>> index 0000000000..e98e4ac8ef
>> --- /dev/null
>> +++ b/arch/x86/cpu/queensbay/cpu.c
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Copyright (C) 2016, Bachmann electronic GmbH
>
> nits: 2018?
>

This change was made some months ago :) But yeah.. can change it.

>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <cpu.h>
>> +#include <dm.h>
>> +#include <asm/cpu.h>
>> +#include <asm/cpu_x86.h>
>> +#include <asm/msr.h>
>> +
>> +static void set_max_freq(void)
>> +{
>> +       msr_t msr;
>> +
>> +       msr = msr_read(MSR_IA32_MISC_ENABLES);
>> +       msr.lo |= (1 << 16);
>
> Please use macro instead of magic numbers

ok

>
>> +       msr_write(MSR_IA32_MISC_ENABLES, msr);
>> +
>> +       msr = msr_read(MSR_IA32_PERF_CTL);
>> +       msr.lo = 0x101f;
>
> ditto

ok

>
>> +       msr_write(MSR_IA32_PERF_CTL, msr);
>> +}
>> +
>> +static int cpu_x86_queensbay_probe(struct udevice *dev)
>
> Queensbay is the platform codename, not the core codename. Queensbay =
> TunnelCreek (the processor) + Topcliff (the bridge chipset).
>
>> +{
>> +       if (!ll_boot_init())
>> +               return 0;
>> +       debug("Init Queensbay core\n");
>
> Should be "TunnelCreek"
>

ok

>> +
>> +       /* Set core to max frequency ratio */
>> +       set_max_freq();
>> +
>> +       return 0;
>> +}
>> +
>> +static int queensbay_get_count(struct udevice *dev)
>> +{
>> +       return 2;
>> +}
>
> Please use cpu_x86_get_count() instead of hard code.
>

That one uses the dts as basis for this information and I need to
add two cpu nodes. I also thought about using cpuid for this. Not
sure what is better.

>> +
>> +static const struct cpu_ops cpu_x86_queensbay_ops = {
>> +       .get_desc       = cpu_x86_get_desc,
>> +       .get_count      = queensbay_get_count,
>> +};
>> +
>> +static const struct udevice_id cpu_x86_queensbay_ids[] = {
>> +       { .compatible = "intel,queensbay-cpu" },
>
> This should be "intel,tunnelcreek-cpu"

ok

>
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(cpu_x86_queensbay_drv) = {
>> +       .name           = "cpu_x86_queensbay",
>> +       .id             = UCLASS_CPU,
>> +       .of_match       = cpu_x86_queensbay_ids,
>> +       .bind           = cpu_x86_bind,
>> +       .probe          = cpu_x86_queensbay_probe,
>> +       .ops            = &cpu_x86_queensbay_ops,
>> +};
>> --

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info