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

[Bin Meng]

Hi Christian,

On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner
<christian.gmeiner at gmail.com> wrote:
> 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

Thank you for the benchmark data. Then please consider describing your
benchmark test suite and on what configuration the data is worse
without this patch (eg: sequential 128-bit read) in the commit
message. This will help other people understand the patch and the
commit in the future.

>
>> 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.
>

I don't understand. Isn't your board U-Boot ported from Intel Crown
Bay? There is arch/x86/dts/crownbay.dts already. You don't need do any
mods in the dts.

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

Regards,
Bin