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

[Christian Gmeiner]

Hi Bin,

2018-04-10 11:07 GMT+02:00 Bin Meng <bmeng.cn at gmail.com>:
> 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.
>

Will do.

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

The dts is not in its ideal state I would say but that is fixable. I
hope to send
out patches for this board soon.

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