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

[Bin Meng]

Hi Christian,

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

Great. Looking forward to your board support patch.

Regards,
Bin